LGTM

The change makes sense..!
Since the patch is more succinct, I'm in favor of applying the patch than this one.

After applying D94859 + D94860 + this patch, the diff looks like this:
https://gist.github.com/aqjune/39bcc99bf7339bef74652597c9e57122 (@nikic thanks for creating the _logical functions!)
I think the majority of sound transformations are now supported.

I'll leave the diff result after setting -instcombine-unsafe-select-transform=0 at another patch that's going to follow this.

LGTM

Address comments, leave tests with FIXME

Leave a comment about the issue

LGTM
Yes, I see that such pattern is quite prevalent. I'll make a patch for this.

Thanks!
Yes, a test should be added.
I guess

ashr <4 x i16> %induction, select (i1 icmp sgt (i16 ptrtoint (i16* @c to i16), i16 1), <4 x i16> zeroinitializer, <4 x i16> <i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16)>)

is the problematic part?
If it is non-trivial to further reduce the input, you can also add this instruction to the list at ValueTrackingTest.cpp 's TEST(ValueTracking, canCreatePoisonOrUndef).

Thank you!

In D92015#2493214, @nikic wrote:

Yes, I think this is fine to land as-is. Note that SDAG always works on a single basic block, so I don't think there is any value in having "branch on undef is UB" semantics, as we can't infer information from branch conditions anyway.

@jdoerfert Would it be okay if I land this after a week? D91480 also uses the semantics described in this patch as well.

In D94386#2490154, @jdoerfert wrote:

Should we add the, "if broken -> UB!" clause while we are here?

In D93065#2486134, @nikic wrote:

@aqjune Do you have any automated way of duplicating the InstCombine tests with logical and/or, similar to what you did for insertelement? Or does this have to be done by hand?

Update

Leave updates in InstCombineVectorOps.cpp only

Address comments

Thinking about @jdoerfert 's suggestion again, it still implies that objects may have overlapping addresses regardless of lifetime. Am I understanding correctly?

It is memset(undef) in other cases

In D92270#2483557, @thakis wrote:

It turned out to be UB in our code as far as I can tell, see https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c36 and the follow-on.

(If this is known to trigger an existing bug more often, it might still be a good idea to undo it until that existing bug is fixed? But we're not affected by this existing bug as far as I can tell, so this is just a suggestion.)

In D93189#2485796, @aqjune wrote:

In other words, is it purely for making analysis flow-insensitive?

I have a question - is it possible to have the same optimization power with !nocapture only? In other words, is it purely for making analysis flow-insensitive?

@efriedma ping

Yeah, I believe passing null to nonnull should not immediately raise UB; it will block useful analyses. The patch is D90529, and I need to push it... Maybe it is time to reduce the number of patches that are still open by me.

To fix the old bug quite a few patches in LLVM have landed so far and it is still ongoing.

In D92270#2482741, @thakis wrote:

We bisected a test failure to this (https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c17). Can you expand a bit on what this patch means in practice? I'm guessing it makes UB in C++ code have bad effects more often? If so, what type of UB?

Resolve failures
A few of those was regressions. It was because ConstantVector::get(Arr) was unable to understand Poison array. I updated those.

In D93943#2476723, @nikic wrote:

Thinking about this again, don't we need to always freeze the extra condition (unless known non-undef/poison)? While poison is only a problem when converting the select form into a branch, doesn't undef need to be frozen even in the and/or form? As the other operand may make it unconditionally true/false and thus well-defined.

In that case, I think we should either always insert the freeze, or not inserted it in the select form either (your original patch) with a FIXME for the more general problem.

LGTM

Use function_ref

Oh yes, I'll look into them

Address comments

In D94061#2478616, @fhahn wrote:

LGTM, as this is aligned with the overall direction we are going.

Fixed, thanks :)

Updates

Rebase

Address comment

The first patch is here: D94002

Minor update

LGTM - Thanks!

In D93993#2476602, @nikic wrote:

LGTM. You might want to update the comment at the top of the file to use poison instead of undef (just to show the canonical IR, doesn't matter semantically).

Rebase
I'll continue splitting after working on lifetime patches

For loop unswitch, I'll work on it after D93764 is done

In D93882#2475917, @nikic wrote:

Something that crossed my mind: While we now handle logical and/or correctly, the same underlying problem also exists for multi-exit loops. In that case we'd combine exit limits from multiple branches via umin(), even though some of them may be poisonous.