Yes, introducing branches on a variable that may be undef/poison is not legal. However, you can use freeze to make it safe.
I think @aqjune fixed loop unswitching already. (don't recall if that was the reverted patch). It's true there a couple more places left to fix.

In D76973#1948751, @spatel wrote:

In D76973#1948172, @fhahn wrote:

We should probably also think about what to do with all the tests that contain branches on undef. Also, bugpoint should probably stop creating branches on undef with this spelled out as is?

When I've updated other tests that would break with changed/improved undef semantics, I usually replace the 'undef' with a parameter like this:
rGfebcb24f1490
Maybe we can script that to fix most of the tests?

LGTM

LGTM (modulo the style fix).
assume poison is UB.

I agree with @aqjune that stating clearly the definition of object in this context.
See this example in the C spec:

For example, the second call of f in g has undefined behavior because each of d[1] through d[49] is accessed through both p and q.

LGTM overall, just some nitpicks.
Something I would add is a link to the list of attributes (is this function-attributes?).

I'm not familiar with the code of this pass, but is there a cheap way of identifying that the two operations are in the same basic block?
If so, you could take the intersection of the aliasing information rather than the union. Because if both ops are guaranteed to execute then the tightest aliasing still has to hold.
(being in the same BB doesn't imply that both instructions are executed, but there's code in ValueTracking perhaps that can check that)

The patch & semantics look good to me, but I'm not a backend expert. I'll leave the final LGTM to someone else.
It would be awesome if we could get this in for 10.0 so that we have complete support for freeze.

@lebedev.ri I agree with you that the semantics of these alignment builtins should only return a pointer that is of the same object as the one given as input.
Otherwise, these builtins would be even worst that ptr2int/int2ptr, since their result could alias with any other pointer in the program, not just the escaped pointers.

LGTM, thanks!

LGTM. The table you provided was very interesting to see, thanks!

In D70749#1767639, @jdoerfert wrote:

You said "Changing the semantics on non-respecting the tags from UB to poison doesn't help either.", could you elaborate why?
If there are no uses of a violating instantiation (undef/null passed to a non-null) we would not get UB but an unused poison value.

I think it's clear that dead arg elimination is incorrect in replacing a valid pointer with null in an attributed with non-null tag. Changing the semantics on non-respecting the tags from UB to poison doesn't help either.
The problem with dropping attributes is that a given function call site, the attributes to be considered are the union of the attributes in the function call and in the callee declaration. We can't rely drop attributes from the callee since some linking later may add them back.

LGTM
Reference to the discussion: http://lists.llvm.org/pipermail/llvm-dev/2019-November/137243.html

The first bit looks ok to me, thanks!

Interesting question :)

Let me give my 2c: eliminating this transformation is a good thing, since it's incorrect (end-to-end miscompilation testcase in the cited bug report).
It can be re-instated if/when we switch from initializing vectors with undef with poison, but I don't know when that will happen. We'll try to push for the poison patches soonish, but it will take time.
Anyway, thanks Sanjay for handling this :)

In D29121#1739974, @RKSimon wrote:

@nlopes This is failing on the llvm-sphinx-docs buildbot, please can you take a look? http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/37793/steps/docs-llvm-html/logs/stdio

Warning, treated as error:
/home/buildbot/llvm-build-dir/llvm-sphinx-docs/llvm/src/llvm/docs/LangRef.rst:10243: WARNING: Could not lex literal_block as "llvm". Highlighting skipped.

In D29011#1735007, @craig.topper wrote:

Does a ConstantExpr freeze really make sense? ConstantExprs are uniqued by the LLVMContext. So every constantexpr that has the same input ends up being the same object. Do we want that behavior do we need each freeze to be distinct?

LGTM!

In D29121#1725727, @reames wrote:

In D29121#1724593, @nlopes wrote:

In D29121#1723978, @jdoerfert wrote:

So this is not committed till we reviewed all of them?

I'm just waiting on SelDAG->MIR patch to be done (https://reviews.llvm.org/D29014).
Then we have docs, SelDAG & MIR support. That seems the minimal to me (so the compiler won't crash when freeze is used). I agree the optimizer patches can land later.

Can I suggest that we add a stub* ISEL implementation, land this, and then build on top of it?

• By which I mean a knowingly incorrect lowering to a simple COPY. It wouldn't fix *all* of our poison/undef problems, but it would allow us to make progress on some of the most painful IR ones while waiting for that patch to land.

In D29121#1723978, @jdoerfert wrote:

So this is not committed till we reviewed all of them?

LGTM

LGTM
(there's only a typo in the comment "singned")

The patch looks good to me. Actually I had reported this bug a while back as well: https://bugs.llvm.org/show_bug.cgi?id=42699
I agree we can't have objects larger than half of the address space.

Clarify semantics for pointers.

In D29121#1696095, @lebedev.ri wrote:

@nlopes anything holding this patch? do you intend to land them all at once?
@aqjune I suppose D29011 should be the next one up for review..

Made it explicit re one vs multiple calls to freeze.

Updated with an example with vectors and add more references to freeze section.

Reflect comments & clarify immediate UB when branching on poison.

In D64215#1577194, @reames wrote:

In D64215#1572285, @nlopes wrote:

In D64215#1572070, @reames wrote:

In D64215#1571424, @nlopes wrote:

Just a shameless plug :)
We've been half secretly working on Alive2 (https://github.com/AliveToolkit/alive2), which includes a plugin for opt that can check if an optimization is correct or not. Alive2 also has a standalone tool that accepts 2 IR files instead.

I'd tried playing with Alive2 a while ago, and had trouble getting it to work. Could you maybe update the readme (or other docs) with some instructions on how to use the standalone tool you mentioned? I'd very much like to play with this.

I've just added a short description to the README file: https://github.com/AliveToolkit/alive2#running-standalone-translation-validation-tool-alive-tv
It's fairly simple: it just takes 2 LLVM IR files. Let me know if you have questions or if you find bugs :)

This tool implements the semantics of poison for many LLVM instructions, and already has some support for memory (which is quite hard to handle).
Of course, what this patch does is not the same. This patch is more executable, while Alive2 requires Z3 to reason about the semantics (though it can also execute code very slowly).

I'd love to explore options for sharing the semantics here. What form does Alive2 express them in?

That's still a unsolved research problem. No one really knows how to share semantics still.
The semantics in Alive2 are written in C++, using an embedded expression language. While it is potentially possible to reuse that somewhere else, it isn't trivial. See e.g. the ir/instr.cpp file.

One thought on sharing.

From what I can tell from a quick look at the code you mentioned, it looks like you're parsing IR into an expression language, then rewriting the expressions to propagate poison - in a fairly similar manner to this code, but over your expression language - and then translating that expression language to SMT. Is that a good high level summary?

In D64215#1574483, @reames wrote:

In D64215#1572285, @nlopes wrote:

In D64215#1572070, @reames wrote:

In D64215#1571424, @nlopes wrote:

Just a shameless plug :)
We've been half secretly working on Alive2 (https://github.com/AliveToolkit/alive2), which includes a plugin for opt that can check if an optimization is correct or not. Alive2 also has a standalone tool that accepts 2 IR files instead.

I'd tried playing with Alive2 a while ago, and had trouble getting it to work. Could you maybe update the readme (or other docs) with some instructions on how to use the standalone tool you mentioned? I'd very much like to play with this.

I've just added a short description to the README file: https://github.com/AliveToolkit/alive2#running-standalone-translation-validation-tool-alive-tv
It's fairly simple: it just takes 2 LLVM IR files. Let me know if you have questions or if you find bugs :)

I finally got it working, required a couple changes to the CMakeFiles and an LD_PRELOAD (for unclear reasons). However, it doesn't look like the scope of the alive-tv tool is anywhere near wide enough for my purposes.

Correct me if I'm wrong, but it looks like it can't handle loops at all right? And use of any memory seams to trigger timeouts? (Even for trivially identical IR?) Just making sure there's no error between keyboard and chair. :)

In D64215#1572070, @reames wrote:

In D64215#1571424, @nlopes wrote:

Just a shameless plug :)
We've been half secretly working on Alive2 (https://github.com/AliveToolkit/alive2), which includes a plugin for opt that can check if an optimization is correct or not. Alive2 also has a standalone tool that accepts 2 IR files instead.

I'd tried playing with Alive2 a while ago, and had trouble getting it to work. Could you maybe update the readme (or other docs) with some instructions on how to use the standalone tool you mentioned? I'd very much like to play with this.

Just a shameless plug :)
We've been half secretly working on Alive2 (https://github.com/AliveToolkit/alive2), which includes a plugin for opt that can check if an optimization is correct or not. Alive2 also has a standalone tool that accepts 2 IR files instead.
This tool implements the semantics of poison for many LLVM instructions, and already has some support for memory (which is quite hard to handle).
Of course, what this patch does is not the same. This patch is more executable, while Alive2 requires Z3 to reason about the semantics (though it can also execute code very slowly).

LGTM, thank you!

Sounds great! Just 1 comment inline.

LGTM.
Minor: you have a typo in the comment: "return return"

I believe the contains function I wrote is correct. It says that an integer n belongs to interval I iff n >= lower(I) and n < upper(I) is there's no wrapping.
BTW, function isWrappedSet() just changed recently, so that needs tweaking in the Z3 model I sent (https://github.com/llvm-mirror/llvm/blob/master/lib/IR/ConstantRange.cpp#L347)

This patch is not correct for this input:
lhs = FullSet
rhs = [0, 1)

In D56155#1342651, @nikic wrote:

Based on the current documentation of poison values, selecting over poison is still poison by the first rule. I'm assuming that this is an inaccuracy in the documentation and selects are treated the same ways as phis? Otherwise the cited clang code would be affected.

As a justification why it doesn't matter whether it's undef or poison, clang emits__builtin_ffs as:
ffs(x) -> x ? cttz(x) + 1 : 0

In D54237#1291405, @nikic wrote:

Regarding

// X + undef -> undef
// undef + X -> undef
if (match(Op1, m_Undef()) || match(Op0, m_Undef()))
  return UndefValue::get(ReturnType);

I was initially planning to include these simplifications, but ultimately was not certain regarding their legality. In particular, if we have uadd.sat(MaxValue, Y), then the result is fully determined to be MaxValue, regardless of the value of Y. If we have something like sadd.sat(SignedMinValue, Y) then the result is known to be negative. In either case the intrinsic cannot have the full range of results of the result type, regardless of the value of Y. As such, I think folding operations on undef to undef would not be legal in this case.

It should be possible to fold uadd.sat(X, undef) to MaxValue. Not sure how useful that is though.

Sure, adding nsw/nuw brings in poison. I didn't study all the MI transformations going on, so I can't comment on whether this is a big burden or not, but without such a study introducing poison is dangerous.
SDAG already has nsw/nuw IIRC, though. But I don't think there was a thorough study of the implications at the time either.

In D49042#1155256, @sanjoy wrote:

Does this mean we can "construct" undef as:

x = alloca 0
y = alloca 0
undef = x == y  // Non-deterministically 0 or 1

? If so, this is a problem since it means even if we spec un-initialized memory as poison we still have undef in the IR (with all the problems it brings).

BTW, I couldn't figure if this case is possible, but mentioning it just in case: it's also not legal to introduce 'sdiv undef, ..', since 'sdiv INT_MIN, -1' is UB. Same for srem. So we need to be careful when folding the shuffle on LHS as well.

In D48893#1152112, @spatel wrote:

In D48893#1151840, @nlopes wrote:

shl %x, undef is poison; we don't want more undefs :)
So this transformation for shifts is not correct. The way to make it correct would be to introduce a poison value and use it in the shuffle instructions instead of undef. I suggest we finally go ahead and do that.

1. Does the 'undef' in the shuffle mask represent something different than the 'undef' in the shift amount?

shl %x, undef is poison; we don't want more undefs :)
So this transformation for shifts is not correct. The way to make it correct would be to introduce a poison value and use it in the shuffle instructions instead of undef. I suggest we finally go ahead and do that.

LGTM

Yes, although hopefully align(4) would have also been specified on the function argument. As the reference must have been bound to a valid object, from C++ semantics, we should know that it's properly aligned. Do we do that now?

I just got a scary ah-ah moment..
Take this code:

The C++ standard defines this as UB: http://eel.is/c++draft/conv.fpint#1
"A prvalue of a floating-point type can be converted to a prvalue of an integer type. The conversion truncates; that is, the fractional part is discarded.
The behavior is undefined if the truncated value cannot be represented in the destination type."

LGTM

Eli, thanks a lot for kicking off the discussion. I think this patch is a bit too big since there are a few things that are not trivial.
For example, I would rather not introduce more functions returning undef, but rather return poison instead. If there's no good motivation for undef, poison should be used by default from now on, since it's much easier to handle than undef.
This patch also introduces a lot of UB with metadata tags, which is a departure from how we handle things like nsw/nuw which make the instructions yield poison instead of UB. Why is it more important to preserve nsw when hoisting an add than preserving !nonnull when hoisting a load? I really don't know; hence I'm asking.
I think it would help to split this patch a bit.

The optimization looks good to me.

LGTM, but I would wait for another review due to the size of the change.

I like the direction in general. I've reviewed this patch and it LGTM (as well as the overall plan).
There are still a few corner cases we need to fix regarding the meaning of size -1, but I guess it's an orthogonal fix. Right now I don't know exactly what -1 size is: does it mean potentially access the whole object, or access the object from the current offset and potentially until the end?

Please don't remove non-functional changes from this patch. It contains dozens of indentation changes which are completely unrelated with the intended change and make the review more difficult. There are also debug printfs included that need to be removed :)
This document may be useful: http://llvm.org/docs/Contributing.html