+1 for landing this patch if there is no significant input from the discourse thread by next Monday (Mar. 13).

In D143287#4179344, @ManuelJBrito wrote:

In D143287#4179020, @aqjune wrote:

H, is D104790 superseded by this patch?

I don't think so we still need to fix the undefined intrinsics, right? Maybe I'm not understanding the question.

H, is D104790 superseded by this patch? I wonder what is the status of this patch as well.

D143129 will do what this patch was supposed to do.

Regarding the updated experimental results, I think people might have thoughts about them. I will leave a comment there.

Thanks..! :)

Hi, I am currently occupied with something else. I will be back by today and continue reviewing this.

ping

LGTM, thanks..!

Thanks! Yes, indeed there are still many files having diffs. But the number shrunk to ~1/5, which seems nice!

We could carve out some exceptions to this bailout. For example, the transform is actually ok with udiv/urem and a common divisor because poison in the dividend would not cause immediate UB - the divisor has to be zero, and that would be UB in the original code too. That doesn't work with signed div/rem because we can choose the poison dividend to be MinSignedValue, and that overflows when divided by -1 causing UB that doesn't exist in the original code.

Allow transformation or types that are to be promoted

Closing this as D142388 added a function that can be used instead

In D144116#4138209, @dmgreen wrote:

Would it be possible to optimize the ADDCARRY to the same result as without this fold? Similar to combineADDCARRYDiamond. I looked at the DAG that was being produced, but it's not obvious to me how it would be sensible combined to the same result as before.

I added the fold really to handle cases like this, which can often come up after lowering geps:

or x1, x1, #1
add x1, x1, x2
ldr x0, [x1]

Which can be transformed into

add x1, x1, x2
ldr x0, [x1, #1]

If the add+add is reassociated, it makes sense for the add+add-like-or to be reassociated. I have no objections to limiting the fold if we need to though.

Reflect comments

Use isMinSignedConstant

The one case I'm probably worried about most are checks of the form %p == null, where replacing %p with null is not generally legal because it has nullary provenance. Replacing two arbitrary pointers likely isn't super important, but replacement with null might be.

Looks good to me in high level, but I suggest waiting for one more accept since I haven't been working with the LLVM codebase for a while.

I checked the updated instructions' LLVM LangRef documentation as well as cppreference.com description and they looked good to me

The clarification looks good to me as well, thanks.
Since LLVM IR does not have a notion of unspecified value (AFAIK), defining these as returning a nondeterministic, well-defined value seems like a right choice because it satisfies refinement.
Making them return an undefined value or poison value breaks refinement.

The updates are analogous to how GNUNullExprClass is processed because UnspecifiedValueExprClass and it is similar (have no operand).

Define undef_or_freeze_undef and use it for the new pattern

The logic looks good to me, but it would be great if the patch gets approvals from people familiar with SelectionDag.

PR31524 (https://github.com/llvm/llvm-project/issues/31524) discusses about the lowering of such intrinsics.
According to the PR, it seems users consider undefined intrinsics as returning unknown but consistent bits.

In D103874#3611483, @nikic wrote:

Which intrinsic are you working on here? If this is about the mm_undefined intrinsics, why do we need to change those from the current status quo of using a zero value instead of undef?

In D103874#3594617, @aqjune wrote:

It seems llvm/lib/Target/X86/X86ISelLowering.cpp's LowerAVXCONCAT_VECTORS is relevant to efficient lowering of shufflevector %x, freeze(poison), mask.

It seems llvm/lib/Target/X86/X86ISelLowering.cpp's LowerAVXCONCAT_VECTORS is relevant to efficient lowering of shufflevector %x, freeze(poison), mask.

Thank you for confirming it!

I suspect that this also obsoletes the CanonicalizeFreezeInLoops pass, and we can probably drop it.

Per LangRef statements of the definition of dereferenceable(<n>) attribute:

The pointer should be well defined, otherwise it is undefined behavior. This means dereferenceable(<n>) implies noundef.

LGTM

Make test show diff, add a description about why @and_noopt is not optimized yet, update comments in ProcessImpliedCondition

Hi, sorry for my delay. The old assertion seems problematic.

In D123473#3484702, @reames wrote:

I don't think we actually want to do that. Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.

In D124526#3483907, @fhahn wrote:

In D124526#3483889, @aqjune wrote:

There was a similar miscompilation report (which was introduced by my patch) and a bug fix due to the existence of select cond, true, false in the past: https://reviews.llvm.org/rG5bb38e84d3d0#986154 and https://reviews.llvm.org/rG6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0
Also a follow-up patch to enhance an assertion condition: https://reviews.llvm.org/rG431a40e1e28f181e87dd247b91a5e6872dd64ab4

This only fixes trivial unswitching for now, but a similar problem
likely exists with non-trivial unswitching.

If the select cond, true, false is a concern, what do you think about simplifying it to cond (https://alive2.llvm.org/ce/z/p5Ahrp) in advance before loop unswitch?

Is it possible you did not check the latest version of the patch? It uses helpers to skip select cons, true, false when trying to match logical and/or.

I don't think we really should simplify/modify the IR if we are not unswitching, which is what 6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0 did, but SimpleLoopUnswitch claims no changes have been made.

In D124677#3483937, @nikic wrote:

In D124677#3483890, @aqjune wrote:

I don't think this follows: This is talking about "integer constant expressions", which are a front-end concern. It means that the front-end is required to match for (void*)0 and emit that as ptr null rather than inttoptr (i64 0 to ptr). At least from that wording, doing int x = 0; (void*)xdoes not result in a null pointer (though possibly other wording implies that?)

I don't think having ptrtoint 1 have universal provenance and ptrtoint 0 have nullary provenance can lead to consistent semantics. It renders many transforms that are "obviously correct" illegal, such as:

define ptr @src(ptr %p, i64 %idx) {
  %p2 = getelementptr i8, ptr %p, i64 %idx
  ret ptr %p2
}
define ptr @tgt(ptr %p, i64 %idx) {
  %p.int = ptrtoint ptr %p to i64
  %p.add = add i64 %p.int, %idx
  %p2 = inttoptr i64 %p.add to ptr
  ret ptr %p2
}

While this transform is very undesirable, it should be correct because it only increases provenance. However, due to the special ptrtoint 0 handling this is incorrect for the special case where p.int == -idx.

This has been observed as a real-world miscompile with rustc.

I agree that introducing ptrtoint + inttoptr here doesn't sound like a good idea because both it is bad for alias analysis and its correctness is not clear.

There was a similar miscompilation report (which was introduced by my patch) and a bug fix due to the existence of select cond, true, false in the past: https://reviews.llvm.org/rG5bb38e84d3d0#986154 and https://reviews.llvm.org/rG6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0
Also a follow-up patch to enhance an assertion condition: https://reviews.llvm.org/rG431a40e1e28f181e87dd247b91a5e6872dd64ab4

Yep, a test added

haveSameProvenanceIfEqual seems like a better name, certainly.

I began to think that we might not need to make replacement of pointers 'directional' because it is too complex.
What about renaming canReplacePointersIfEqual in Loads.h to haveSameProvenance and only checking the equivalence of two pointers, which will make things simpler (including this patch)?

It sounds great - then after the removal of the old pass I will make a patch that removes https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2508 as well.

A quick question: should LoopUnswitch.cpp be fixed as well? It seems the pass is not maintained though. Will SimpleLoopUnswitch supercede the LoopUnswitch pass?

Thank you for your work..! The diff looks fine.

In D123991#3464364, @nlopes wrote:

Well, here we are discussing the semantics of LLVM IR. It's ok for the semantics of SDAG to be different, as long as it's a refinement of LLVM IR's.

I confirmed that there are a few other architectures assuming the same semantics:

• AArch64: https://llvm.godbolt.org/z/b5M5j6r9h
• RISCV32: https://llvm.godbolt.org/z/1ax4s5jPo
• RISCV64: https://llvm.godbolt.org/z/W59oazebx
• X86 (32-bit): https://llvm.godbolt.org/z/s3PTznrxa

Also, could you leave a link to Alive2 in the patch description showing that pushing freeze into icmp ops is safe when there are multiple users?

In D89050#3272837, @malharJ wrote:

Right, but the absence of select -> and/or folding by itself shouldn't matter -- it's only relevant in that it can prevent other transforms

I fully agree, it's just that the presence of that transformation was helping with an optimization later on in the pipeline
in my specific case.

Otherwise there is no issue with code generation of select.

Since x == 0 ? x : umin(x, y) cannot be represented using the current SCEV operations (at least using the ops in SCEVTypes). I believe the new ops in this patch are necessary.
Another approach to support such expressions would be adding a ternary operator and comparisons to SCEV, but it would require bigger changes, I guess?

In D93793#3221414, @Allen wrote:

I have a babyism question, why poison is preferred to the undef in the pattern ConstantVector::getSplat ?

This sounds like a great fix to me! :)

In D115526#3185703, @spatel wrote:

In D115526#3185635, @nikic wrote:

It looks like the shufflevector poison change hasn't landed yet? D93818 / D103874

Thanks for the reminder! ( cc @aqjune @hyeongyukim @regehr )