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 )

Hello all,
I couldn't find this mail because it was somehow buried in other mails.

In D105169#3104262, @eugenis wrote:

You are absolutely right. X86 variant uses an "=a" constraint (rax register), others pin the output variable to a specific register with asm declaration. It appears we've missed it in Aarch64.

Could you check if asm("x0") in the declaration of res helps?

I am not familiar with inline assembly, but it seems the output variable (%0) is not updated because it does not appear in the code.

I will revert this patch, since its fix needs some time for me to investigate.
I have a colleague who has access to an AArch64 server, so I can give it a try by myself.

In D105169#3069417, @mstorsjo wrote:

Yes, I believe so. If the branch is not taken, pos_min and pos_max are undefined when entering ff_gen_search. (I would assume that their value isn't ever used within ff_gen_search in that case.) But regardless of that, in this case, the generated code crashes around this line, https://gist.github.com/aqjune/3bd0ea19bbc12b4744843c0c070e994c#file-ff_seek_frame_binary-c-L39, before entering ff_gen_search - and within that branch, those variables are properly set before they're used.

It seems the original code has a use of an uninitialized variable.
Line 4420 at seek-preproc.c (function ff_seek_frame_binary):

 int64_t pos_min=pos_min, pos_max=pos_max, pos, pos_limit; // pos_min and pos_max are self-assigned.
...
if (sti->index_entries) {
   ...
}
// pos_min and pos_max are used as arguments below
pos = ff_gen_search(s, stream_index, target_ts, pos_min, pos_max, pos_limit,
                     ts_min, ts_max, flags, &ts, avif->read_timestamp);

I can see that @ff_seek_frame_binary is the only affected function.
It introduces llvm.assume as well as !nonnull at a few places and folds null pointer checks.
Still investigating..

There has been no concern in a week, so I'd like to land this patch and D108453 in this weekend.
I'll carefully watch the buildbots and address failures if any.
@eugenis could you accept this patch and D108453 please, if you don't mind?

I thought about the validity of this transformation.
To be pedantic, the validity is unclear when ptr is
(1) an out-of-bounds pointer such that (intptr_t)ptr is 0.
But, I remember people wanted to regard free(ptr) as equivalent to free(null) in that case, so it would be fine.
(2) a partially undefined pointer s.t. ptr = undef | 1
But... I couldn't see any such expression appear in practice, and I don't think LLVM is strictly correct with respect to partially undef pointers

The change looks great to me.

Thank you!

LGTM

@spatel Thanks!
@hyeongyukim Do you mind if we add the new CreateInsertElements and use them in this patch?

Explicitly state that undef can be given as a shuffle mask and its meaning is equivalent to that of the poison mask

Resurrect mistakenly removed test statements

Rebase

I abandon this patch because (1) shufflevector parts are covered in D110226, D110227, D110230, and (2) insertelement parts will be covered in upcoming new patches.

I abandon this patch because (1) shufflevector parts are covered in D110226, D110227, D110230, and (2) insertelement parts will be covered in upcoming new patches.

I left comments on a few changes - I think it is okay to leave ones that are clearly valid if you prefer.

Hello all, I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default.
The motivation and background of these patches are described at https://reviews.llvm.org/D105169#2959464.

Hello all,

Rebase, rename disable-noundef-args to disable-noundef-analysis, remove redundant diffs

Rename disable-noundef-args to disable-noundef-analysis

In terms of correctness, I think the patch looks okay.

I became to wonder what happens if a pointer is freed in the function.
For example:

// assume that %p is dereferenceable(8)
call void @g(%p);
%q = inttoptr (ptrtoint %p);
use(%q); // can we replace this with %p?

LGTM, thank you for writing this patch.
Since the poison values in the insertelement and shufflevector are not visible from shufflevector's users, I believe this change is harmless.

I think so? it sounds great.
Could you try inserting freeze to icmp only when one of its operands is constant?
ex) freeze (icmp op0, 1) -> icmp (freeze(op0), 1)
ex2) freeze (icmp op0, op1) -/-> icmp (freeze(op0), freeze(op1)) <= this splits one freeze into two; its benefit is not clear.

I left a few nitpicks that is probably worth addressed

LGTM.

If loop unswitch introduced the freeze, what about adding an extra InstCombine pass between the loop unswitch and SimplifyCFG?

As suggested, finding a common dominator and sinking the instruction to the location seems reasonable and worth trying.
It will help reduce the diff at line 3866~, I hope.
If DT is null, fallback to the old one use check?

Just found that this old patch is still open - I'd like to revisit this later since freeze can be used to resurrect this transformation. This will require investigation into the impact of freeze.

LGTM too.

In D106289#2916096, @lebedev.ri wrote:

In D106289#2916089, @aqjune wrote:

I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?

I'm going to guess no. inttoptr/ptrtoint are essentially no-op bitcasts,
while addrspacecast is implementation-defined and in general case can be assumed to e.g. offset the pointer.

I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?

Since there is a second opinion about using TBAA, will close this for now.