I made https://reviews.llvm.org/D100122 to show how the patch would look like if m_Undef is fixed instead.

In D99853#2667983, @nikic wrote:

This problem isn't really limited to this one function in InstSimplify. Might it make sense to adjust UndefValue to actually represent such constants? We'd have to add a bitmask to it that indicates which values are poison.

Thank you all! Incremental change makes sense to me as well. It will help smooth landing without buildbot failures.
I'll start writing patches soon.

LGTM, Thanks!

In D99642#2674663, @fhahn wrote:

LGTM, thanks!

@nlopes do you have any more thoughts on the difference between LLVM & Alive2 on this topic?

This tracks down to a commit in 2010: https://github.com/llvm/llvm-project/commit/d9df6eaa9ce20
Interestingly the motivation of introducing ConstantPreference was to properly deal with indirectbr. :)
I have no strong preference either.

This turns out to be a bug in InstCombine. I reported it as https://llvm.org/pr49832 .

In D93990#2666922, @fhahn wrote:

Hi! It looks like this may be causing https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29445 .

rebase, clang-format

In D99674#2667634, @nikic wrote:

I'm OK with landing this first to fix an active miscompile, but I don't think we should block flipping the switch on noundef work. I think your numbers support that we shouldn't wait on that, because the noundef impact it small relative to the whole change (it only makes a 15% difference in affected files).

rebase, correctify the test file name

In D99674#2662023, @nikic wrote:

Unconditionally disabling this transformation affects later pipelines that depend on and/or i1s.
To minimize its impact, this patch conservatively checks whether cond2 is an instruction that
creates a poison or its operand creates a poison.

Do you have any further information on the regressions this would result in?

Given all the preliminary work that has gone into this, I'd really like to have the option flipped outright, and not introduce another hack.

It indeed seems tricky to find out the root cause for this; this optimization enabled vectorization which led to the error for some reason.
I'll have more time to investigate this tomorrow and weekends.

In rG5bb38e84d3d0420e7159ffc34e3dd33ab5088b8e#986321, @DaniilSuchkov wrote:

In rG5bb38e84d3d0420e7159ffc34e3dd33ab5088b8e#986266, @aqjune wrote:

I made a fix to keep the main branch in a good state, but not sure this is a real solution because there might be a hidden problem. I'm investigating it.

Please add an assertion that m_LogicalOr() and m_LogicalAnd() aren't true at the same time.

I made a fix to keep the main branch in a good state, but not sure this is a real solution because there might be a hidden problem. I'm investigating it.

I'll make a fix for this, thanks.

Hello all,
Is there a plan to enable -enable-noundef-analysis by default? It seems this patch is already addressing a lot of test cases.
Another good news is that DeadArgElim is also fixed by D98899 to correctly drop UB-raising attributes such as noundef when replacing an unused argument with undef. This was an issue which is known to be problematic when the flag was activated.
I can make a patch based on this work instead if people want.

gently ping

ping

The core notions here are: 1) separate dereferenceability and object lifetime (in the marker intrinsic sense), and 2) unify handling for all memory object types.

Closed in favor of D99027

Closed in favor of D99027

Should be fixed via https://reviews.llvm.org/rGb00209ed100c

It is impressive that leaving the few patterns was enough to cover all existing unit tests.
If more patterns are needed, we can factor out the non-refined foldings from SimplifyXXInst as a static function and call it here.

In D85534#2636321, @Jim wrote:

Hi,
This flag is enabled in LTO. I met a failure on my test program.
It shows

Unsupported instruction encountered
llvm/lib/Analysis/CFLGraph.h:260!

It looks like that CFLGraph is unable to support FreezeInst.
Could you give me some suggesion to fix it?
Thanks

In terms of maintenance: if reviewers think the complexity this patch is introducing is big to maintain, I'll turn this patch to using canCreatePoison check since it will fix the bug anyway.

Does this direction look okay? @nikic
One benefit of this approach is that it detects unsafe replacements that have non-poison-creating instructions as well.
For example, this transformation is illegal:

  %v = select (x == 0), 0, (x & y)
=>
  %v = x & y

If x = 0 and y was poison this makes the result more undefined. However, the current check relying on canCreatePoison cannot detect this.
D98585 fixes this by making the relevant transformation respect Q.AllowRefinement.

the fact that we now have "multiple kinds of arguments" is confusing.

update

Address comments, minor update to the grammar

update

Thank you all!

check if the argument has noundef

To be safe, what do you think about marking nosync to ops that can be represented as a series of loads/stores or scalar ops only? For example, I believe memset is nosync because it is equivalent to a series of nonatomic stores.
For side-effecting operations, such as printf, I'm not 100% sure whether it is nosync. printf interacts with cout to properly flush buffers, which might do some interactions.

Make computePointerICmp slightly more conservative if Q.AllowRefinement is false

In D98228#2613304, @Tyker wrote:

I think the process of checking is a value is noundef needs to be more general than just calls. maybe addKnowledge would be a better place.
because nonnull and align information can also be inferred from loads/stores.

minor updates

Add AllowRefinement field

TBH, I don't feel comfortable with extending the canCreatePoison() check. :( Even if it requires inspection of foldings in InstSimplify, I think a safer way is to add a flag that disallows foldings returning refined values.
Let me update this patch and write following patches for updating them.

In D98410#2619315, @bjope wrote:

In D98410#2619250, @nagisa wrote:

Wouldn't folding C * undef and undef * C into an undef, rather than 0, be valid/better? Is there a reason we cannot do that?

I figure that it would be wrong if for example C is zero, because then the result can't take any other value than zero. Or if C is 2 and scale is 0, then the result can't be odd. So undef include values that isn't possible for every combination of the other operands.

The poison propagation is a bit more new for me. I hope I got that part correct.

In D90529#2613303, @Tyker wrote:

In D90529#2612794, @aqjune wrote:

A fix in AssumeBundleBuilder to make it comply LangRef: D98228

BTW, I found that "align" can take two operands: "align"(i8* ptr, i64 a, i64 b) What is the meaning of the second index (b)?

the meaning of the second index is the offset of the alignment. so with "align"(i8* ptr, i64 16, i64 12), ptr is aligned on 4 but ptr + 4 is aligned on 16.
I introduced this to support __builtin_assume_aligned which has similar semantics.

Confirmed that this resolves the regression (https://reviews.llvm.org/D95026#2610334) from two-stage build!

ping

Well, I found a way to get around this issue without changing these APIs. :)

Fix deoptimized case