In rG3cdd05e519ddab7d4e4864cadb977a876cd19046#1003214, @spatel wrote:

I pushed a minimal fix (intersect 'nsz'):
d2012d965d60

In D99481#2790025, @nathanchance wrote:

This change breaks building the Linux kernel:

I think this test case isn't correct when %b is undef:

define <2 x float> @select_fneg_false(<2 x float> %x, <2 x float> %y, <2 x i1> %b) {
  %ny = fneg nnan <2 x float> %y
  %s = select ninf <2 x i1> %b, <2 x float> %x, <2 x float> %ny
  %r = fneg nnan nsz <2 x float> %s
  ret <2 x float> %r
}
=>
define <2 x float> @select_fneg_false(<2 x float> %x, <2 x float> %y, <2 x i1> %b) {
  %x.neg = fneg nnan nsz <2 x float> %x
  %1 = select nnan nsz <2 x i1> %b, <2 x float> %x.neg, <2 x float> %y
  ret <2 x float> %1
}

LGTM, thanks!

Sorry, missed the poison case. Just look at the daily Alive2 run and it failed this test:

define void @insert_store_nonconst_index_known_valid_by_and(* %q, i8 %s, i32 %idx) {
%entry:
  %0 = load <16 x i8>, * %q, align 16
  %idx.clamped = and i32 %idx, 7
  %vecins = insertelement <16 x i8> %0, i8 %s, i32 %idx.clamped
  store <16 x i8> %vecins, * %q, align 16
  ret void
}
=>
define void @insert_store_nonconst_index_known_valid_by_and(* %q, i8 %s, i32 %idx) {
%entry:
  %idx.clamped = and i32 %idx, 7
  %0 = gep inbounds * %q, 16 x i32 0, 1 x i32 %idx.clamped
  store i8 %s, * %0, align 1
  ret void
}
Transformation doesn't verify!
ERROR: Source is more defined than target

In D102476#2779213, @fhahn wrote:

In D102476#2777021, @nlopes wrote:

This patch looks good if we want to go with the poison-on-OOB semantics.
I think it's important to settle that first, please see my bug report: https://bugs.llvm.org/show_bug.cgi?id=50382

I think the conclusion of the discussion at the patch to far seems to be to keep it poison. So I think we could move ahead with this patch?

This patch looks good if we want to go with the poison-on-OOB semantics.
I think it's important to settle that first, please see my bug report: https://bugs.llvm.org/show_bug.cgi?id=50382
The backend does this very same optimization but without paying attention to whether the index is in-bounds or not. Which effectively makes extractvalue similar to a load instruction, including all the restrictions around movement and so on. Is that what we want?
We need to either fix the backend and move on with this patch (poison on OOB), or fix langref and abandon this patch (UB on OOB). Since I don't really know which frontend produces extractelement and from which code, I don't have an opinion either way, although usually UB is not a good idea.

In D101727#2759617, @spatel wrote:

In D101727#2746020, @nlopes wrote:

FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Great! Is it available on the online instance yet?
Checking my understanding on this example:
https://alive2.llvm.org/ce/z/M2Fe49

define double @src(double %x) {
%0:
  %lezero = fcmp ole double %x, 0.000000
  %negx = fsub nnan double 0.000000, %x
  %fabs = select i1 %lezero, double %negx, double %x
  ret double %fabs
}
=>
define double @tgt(double %x) {
%0:
  %fabs = fabs nnan double %x
  ret double %fabs
}
Transformation doesn't verify!

ERROR: Target is more poisonous than source

Example:
double %x = NaN

Source:
i1 %lezero = #x0 (0)
double %negx = poison
double %fabs = NaN

Target:
double %fabs = poison
Source value: NaN
Target value: poison

In the source function, if we have "nnan" on the select, and we choose %x (NaN), then the return value must be poison? The "nnan" on the select was dropped in the output pane though?

In D102292#2754879, @aeubanks wrote:

In D102292#2753579, @nlopes wrote:

Can you clarify if this only for data type or also for function types pls?
(data and function pointer types don't have the same bit-width on all architectures)

This should apply to all pointer types.
Do you have an example in LLVM of where this matters for function vs data?
In TargetMachine I only see the pointer size differences per address space, not function vs data. TargetMachine::getPointerSize()/DataLayout::getPointerSize().

In D101701#2754739, @jdoerfert wrote:

In D101701#2754648, @nlopes wrote:

3. Today's nofree enables annotations and optimizations that the proposed change would no longer allow. I can't say for certain whether this is false, but at least the examples given so far seem to fail (the first one because you can just use the nofree argument attribute instead, and the second is discussed above in this comment).

I think once I manage to explain the examples you will reconsider this point. To give you another one:

__attribute__((nosync)) // or __attribute__((assume("nosync")) or #pragma omp assume ext_no_synchronization
void user_code() {
  a = malloc();
  b = malloc();
  unknown_function_with_a_now_nosync_call_site(a, b);
  ...
  free(a);
}

This is a good example, that shows that if the user annotates a function with nosync then inferring nofree becomes easier. So the proposed semantics doesn't require nosync, but inference can be aided by its presence (not in this patch yet).

This is not at all what the example shows. The example shows that we cannot infer nofree for unknown_function_with_a_now_nosync_call_site with the new semantics but with the old semantics we could.
Only with the existing semantics you can optimize this example, not with the new one. I would have assumed this is implied given that I provided the example and I am arguing the proposed semantics are useless and prevent optimizations.

3. Today's nofree enables annotations and optimizations that the proposed change would no longer allow. I can't say for certain whether this is false, but at least the examples given so far seem to fail (the first one because you can just use the nofree argument attribute instead, and the second is discussed above in this comment).

I think once I manage to explain the examples you will reconsider this point. To give you another one:

__attribute__((nosync)) // or __attribute__((assume("nosync")) or #pragma omp assume ext_no_synchronization
void user_code() {
  a = malloc();
  b = malloc();
  unknown_function_with_a_now_nosync_call_site(a, b);
  ...
  free(a);
}

Can you clarify if this only for data type or also for function types pls?
(data and function pointer types don't have the same bit-width on all architectures)

In D101701#2746949, @jdoerfert wrote:

In D101701#2746841, @nlopes wrote:

@jdoerfert a function that syncs with another thread may not free anything itself and the other thread may not free anything either.
If you require an optimization to have nofree + nosync in a call to preserve dereferenceability then you can't optimize this case.

I disagree, and I think this is the conceptual differences we have here.

If one does only look at a function in isolation then I agree, nofree alone is not sufficient to make statements about all call sites and arguments.
However, if you look at a call site of a nofree function the situation is different because you have context information about the arguments.

Let's go back to my last example:

a = malloc
b = malloc
unknown(a);                  // no attributes 0-> escapes
nofree_nocapture_only(a, b); // `nofree`, `nocapture` on args

I know that at the end of this sequence b has not been freed, even though there is no nosync in sight.
I can also derive nofree for nofree_nocapture_only without worrying about atomics, for example.
(Here is the sequence in action with heap2stack kicking in for b: https://godbolt.org/z/hjYrhaEvW)

So, with nofree and nosync being completely separated, as it was in the very beginning, you can:

1. Optimize even if only one is present, given additional call site information.
2. Derive nofree even if nosync is not present.

@jdoerfert a function that syncs with another thread may not free anything itself and the other thread may not free anything either.
If you require an optimization to have nofree + nosync in a call to preserve dereferenceability then you can't optimize this case.

This commit needs to be reverted; it's fundamentally flawed.

FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Amazing, thank you!

LGTM for the LangRef changes.
I agree the proposed semantics is more useful this way. Plus I agree with the argument that the IR is more expressive this way, as it then supports nofree functions that do atomic stuff.

There's this comment in isNonEscapingLocalObject:

// If this is an argument that corresponds to a byval or noalias argument,
// then it has not escaped before entering the function.  Check if it escapes
// inside the function.

I'm not a fan of this semantics either. nofree should mean nofree (transitively).
A nofree function shouldn't be able to pass a pointer to another thread and get it freed.

I've updated Alive2 with the new semantics and I see one regression:
llvm/test/Transforms/InstCombine/malloc-free-delete.ll

It looks great to me!

LGTM

LGTM

In D69428#2687372, @saugustine wrote:

I went ahead and reverted. We can discuss a plan--hopefully the internal cleanup to obviate this will go smoothly.

Let me know if there are more issues.

@aqjune ping: there are a few sentences that need tweaking.

Please go ahead. The relevant stakeholders didn't reply, so let's assume they are not interested in this functionality anymore.
Anyway, these days people can use @llvm.used if needed.

In D99642#2674663, @fhahn wrote:

LGTM, thanks!

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

In rGef1f90ba6761f5fc3e7fac8652214f74cbf87c20#987171, @ABataev wrote:

It should be fixed by https://reviews.llvm.org/rG5fcb07a07020 already.

Is this bug being tracked somewhere? Alive2 complains about this:

@arr = global 128 bytes, align 16
@var = global 4 bytes, align 8

In D99642#2660961, @fhahn wrote:

In D99642#2660952, @nlopes wrote:

In D99642#2660864, @lebedev.ri wrote:

In D99642#2660849, @fhahn wrote:

In D99642#2660824, @lebedev.ri wrote:

alive2 again doesn't agree that non-inbounds GEP is allowed to produce null pointer: https://alive2.llvm.org/ce/z/9wfL5x

Interesting, but also surprising, especially because the LangRef explicitly calls out GEPs without inbounds to wrap silently?

Yes.
I think it's another example of alive2 trying to invent a memory model that isn't fit for the real world.

Let me try to explain our reasoning:

• GEP doesn't change the object of a pointer, just the offset within that object. Having this rule enables a lot of optimizations (e.g. you know "for free" that gep %p, %x and gep %q, %y can't alias if %p/%q are the result of e.g. distinct alloca/malloc, even if you know nothing about %x/%y).

I think that's fine for AA, which generally looks at pointers that are dereferenced. It seems a bit unfortunate though that we determine that the pointer is based on object but still may not be de-referenceable if the object is though.

In D99642#2660864, @lebedev.ri wrote:

In D99642#2660849, @fhahn wrote:

In D99642#2660824, @lebedev.ri wrote:

alive2 again doesn't agree that non-inbounds GEP is allowed to produce null pointer: https://alive2.llvm.org/ce/z/9wfL5x

Interesting, but also surprising, especially because the LangRef explicitly calls out GEPs without inbounds to wrap silently?

Yes.
I think it's another example of alive2 trying to invent a memory model that isn't fit for the real world.

In D69428#2658010, @evgeny777 wrote:

@nlopes

Not sure you are still interested in this patch. If so, I would suggest you get in touch with some Google folks and check with them if their codebase is ready for this patch. They were the only reason for this workaround.

Yeah, it would be nice to get rid of this valgrind legacy (btw, there is already D70006 which eliminates such kind of unneeded globals for thin LTO). However I don't whom to contact.

Not sure you are still interested in this patch. If so, I would suggest you get in touch with some Google folks and check with them if their codebase is ready for this patch. They were the only reason for this workaround.

Given that it gives a decent speedup for some important workload, and provided it doesn't regress others, I think this should go in then.
It's easy to revert this once opaque pointers arrive.

In D99121#2645593, @dblaikie wrote:

In D99121#2644362, @lebedev.ri wrote:

In D99121#2644223, @nlopes wrote:

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.

Yep. That will be indeed a great to see.

So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

It is indeed temporary until Opaque pointers are here.
The problem has been stated last time in D99051 by @ruiling:
https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
there can be any number of pointers inttoptr'd from it,
and passes won't be able to tell that they are identical.

@dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? Is there a checklist somewhere?

no checklist, unfortunately - myself, @t.p.northover, @jyknight, and @arsenm have all done bits and pieces of work on it lately.

I think we've got most of the big IR changes (adding explicit types where they'll be needed when they're no longer carried on the type of pointer parameters) - @arsenm's D98146 is another piece in that area, hopefully near the last I think.

After all that's in place, the next step I think would be to introduce the typeless pointer, support it as an operand to these various operations - and then try producing it as a result of instructions too. But I'm probably missing a bunch of important steps we'll find are necessary...

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.
So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

In D98179#2639565, @davezarzycki wrote:

In D98179#2639554, @nlopes wrote:

In D98179#2639491, @davezarzycki wrote:

In D98179#2639476, @nlopes wrote:

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

That still sounds incredibly brittle. If there is any variety in test machine performance, then any and all attempts at sorting should be futile because the underlying hardware will perturb different timeouts. Is this not your experience? How do you reconcile hardware performance and configuration details (like SMT) with timeout settings?

Of course it's brittle :) Changing from a time-based setting to a ticks-based system is ongoing work, such that resource exhaustion becomes deterministic.
Nevertheless, on a same machine, we don't see many test flips. It's quite stable most of the times (just one test flip once in a while).

This seems really beyond the scope and purpose of sorting the tests.

If you don't mind and given that the workaround is trivial (delete the timing data), I'd like to hold off on adding --ignore-timing-data. If enough people complain then we can add that option. Is that okay with you?

In D98179#2639491, @davezarzycki wrote:

In D98179#2639476, @nlopes wrote:

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

That still sounds incredibly brittle. If there is any variety in test machine performance, then any and all attempts at sorting should be futile because the underlying hardware will perturb different timeouts. Is this not your experience? How do you reconcile hardware performance and configuration details (like SMT) with timeout settings?

Thank you!

I'm talking about sorting just the summary of failed tests, not the whole output. We need the whole -vv output, but that can be out of order.

In D98179#2638801, @mehdi_amini wrote:

Can we revert to the previous behavior please? The current behavior is not user friendly. Thanks!

To clarify: you care about the order in the final summary, not the actual execution order, right? (the goal of this patch is the latter, if it changes the former this is just a side-effect I believe)

This patch makes the order of the list of failing tests non-deterministic. This is extremely annoying because you can't do a simple diff between test dumps anymore.
Before the list of failed tests used to be sorted.

It is LGTM, yes.
Thanks for the details on the plan. Sounds great, thanks!

Sounds good to me, as long as you commit to removing that cmd switch within a reasonable time frame. Otherwise we accumulate technical debt.

Thanks for expanding on the padding. Sounds good to me.
LGTM.

Overall LGTM. Thanks for documenting this! It was painful to reverse-engineer this when implementing it in Alive2..

In D97924#2611453, @reames wrote:

In D97924#2607261, @nlopes wrote:

Consider this example:

f(nocapture p) {
  p2 = load glb
  ret p2
}

According to your definition, this functions triggers UB if p2 == p, because it increases the number of observations of p. It's counter-intuitive to me that a function that doesn't touch its nocapture argument captures that argument.

I agree, this is slightly counter intuitive. I think it's also fundamentally necessary.

In D97924#2607361, @jdoerfert wrote:

// G is only used in this function and always written first, no leakage!
static int *G;
void noescape1(int *p) {
    int q;
    G = cond() ? p : &q;
    *G = 3;
}

If we have an object which has not yet been captured passed to a nocapture argument of a function, we know that the object remains uncaptured after the call. Additionally, we also know that the callee hasn't increased the number of locations in which references to the object can be observed after the call. Thus, if the caller can precisely enumerate said set before the call, that said remains precise after the call completes.

In D94002#2606276, @RalfJung wrote:

The exact semantics of lifetime.start depends on the pattern matching patterns in the stack coloring algorithm. So this intrinsic cannot be abused. It must be used for the uses it was created for only.

That's fair, but then shouldn't the docs say that? Usually one would expect the docs to say everything there is to be said; so in this case a dedicated warning might be in order to document the caveats you just mentioned.

@rsmith already gave his blessing, so please go ahead.

We've had a several months-long discussion on this topic. I think we've reached quorum to move forward.
The patch looks great, thanks for your work. Please go ahead and commit it!

In D88287#2589841, @ebrevnov wrote:

I think the solution is to use >= instead of > when we do min/max reassociation. In other words, originally we had 'any' > MAX_INT which is known to be false. If we want semantically equal but reassociated expression we should invert the comparison logic. In other words we should check MAX_INT >= 'any' which is known to be true and MAX_INT will be selected.

In D88287#2589378, @ebrevnov wrote:

I think this is an issue of verification itself. In the first case max(0, undef)=>any and max(any, max_int)=>max_int. In the second case max(max_int, undef)=>x03002006. I believe the behavior of the verifier is inconsistent in these two cases and max(max_int, undef) should be evaluated to max_int as well. We can do the following trivial transformations to prove that: max(max_int, undef) is trivially equal to max(max_int, max(undef, undef)) and max(undef, undef) should be evaluated to 'any' since max(0, undef) is evaluated to 'any' in the first case. Thus we get max(max_int, any) which is evaluated to 'max_int' in the first case. So max(max_int, undef) should be evaluated to 'max_int' but not 'x03002006'.

Makes sense?

This patch regressed the following tests:

• LLVM :: Transforms/NaryReassociate/nary-smax.ll
• LLVM :: Transforms/NaryReassociate/nary-smin.ll
• LLVM :: Transforms/NaryReassociate/nary-umax.ll
• LLVM :: Transforms/NaryReassociate/nary-umin.ll

In D94014#2490334, @spatel wrote:

Anyone see problems with this Alive2 implementation using count-leading-*?
https://alive2.llvm.org/ce/z/SWxadd

I also manually entered all of the i4 regression tests with fixed constants in Alive1 (rise4fun), and they appear to be correct as shown in the test diffs.

In D94014#2490258, @spatel wrote:

In D94014#2490113, @lebedev.ri wrote:

alive1 does not actually have a countLeadingOnes() precondition

Weird - maybe @nlopes can tell us how this example parses at all then: https://rise4fun.com/Alive/juX1

(at least as per https://github.com/nunoplopes/alive/blob/master/constants.py),

In D93820#2489066, @nikic wrote:

Based on what @RalfJung mentioned on zulip, the question of whether the transform is legal for inbounds comes down to the particular choice of inbounds semantics. I was using the semantics specified in LangRef, which make the optimization illegal, while @nlopes used the semantics from https://people.mpi-sws.org/~jung/twinsem/twinsem.pdf (or something similar), which makes it legal. The relevant difference to the LangRef semantics (if we stick to the gep-inbounds-logical case) would be:

- The base pointer has an in bounds address of an allocated object [...]
+ The base pointer has an in bounds address of the allocated object it is based on [...]

In any case, regardless of whether this is legal for the inbounds case, I think everyone agrees it's not legal for the non-inbounds case (and not legal for the non-null case regardless of inbounds). Is that enough to move forward here, or do you want me to thread inbounds information through SimplifyGEPInst and retain this optimization for the inbounds case?

In D89697#2484919, @spatel wrote:

In D89697#2483676, @yubing wrote:

No regression appeared in our internal testcases.
It seems the transform is correct, have you verified it with alive-tv?

I was curious to see if I could model it:
https://alive2.llvm.org/ce/z/RXcYY9

Converting #x4f800000 (4294967296) to uint32_t is poison, not 0 though. Am I reading the Alive output correctly? (cc @lebedev.ri @aqjune @nlopes @nikic )

In D89697#2484919, @spatel wrote:

In D89697#2483676, @yubing wrote:

No regression appeared in our internal testcases.
It seems the transform is correct, have you verified it with alive-tv?

I was curious to see if I could model it:
https://alive2.llvm.org/ce/z/RXcYY9

Converting #x4f800000 (4294967296) to uint32_t is poison, not 0 though. Am I reading the Alive output correctly? (cc @lebedev.ri @aqjune @nlopes @nikic )

In D87188#2476230, @thakis wrote:

In D87188#2470401, @lebedev.ri wrote:

In D87188#2470392, @thakis wrote:

Heads up: Breaks a test for us: https://bugs.chromium.org/p/chromium/issues/detail?id=1161542

(No reduced repro yet, might be UB, just fyi at this point.)

Thanks for headsup. For now i'll deal with the problem @nlopes pointed out above in a bit..

Just to follow up, this ended up being UB on our end (fix: https://android-review.googlesource.com/c/platform/external/perfetto/+/1535483)

In rGa2513cb8655e0aea4baffb4391e946ad3e56d883#967810, @AndrewLitteken wrote:

I reverted this commit because it was causing problems with clang on several of the buildbots with the general flavor of the following error log:

http://lab.llvm.org:8011/#/builders/21/builds/5863/steps/5/logs/stdio

llvm-project/llvm/include/llvm/ExecutionEngine/Orc/Shared/RPCUtils.h:1519:14: error: call to deleted constructor of 'llvm::Error'
      return Err;
             ^~~

In where the changes were made and:

llvm-project/llvm/include/llvm/ExecutionEngine/Orc/Shared/RPCUtils.h:1232:29: error: no matching member function for call to 'callB'
              Impl.template callB<OrcRPCNegotiate>(Func::getPrototype())) {

In D93820#2471805, @nikic wrote:

In D93820#2471788, @nlopes wrote:

In D93820#2471778, @nikic wrote:

In D93820#2471737, @nlopes wrote:

I only looked at the tests and they were correct before, see here: https://alive2.llvm.org/ce/z/UzW3pv
The tests are weird because they have 'gep inbounds'. The reason they are correct (and weird) is that the only way p - (int)p/sizeof(*p) is inbounds is p being null. Anything else will overflow.

This doesn't look right to me, at least not given current LangRef wording. Lets say we have gep inbounds p, -p, where p = ptr(base_addr = 1, offset = -1). This means that the address value of p is 0, but it has provenance of the object at base_addr = 1. As such, the inbounds is not violated (both p and the gep results are inbounds of the zero address), but we still change provenance.

There's an extra catch: gep inbounds requires both the input and output pointers to be in bounds. This part is explicit in LangRef, at least.
Some examples:

p = malloc()
q = gep inbounds p, -1  // poison
r = gep p, -1           // ok
s = gep inbounds r, 1   // poison: r is not inbounds
t = gep r, 1            // ok, offset = 0
u = gep inbounds t, 1   // ok, offset = 1 (assuming malloc size > 0)

Right, but inbounds and provenance are, as far as I can tell, orthogonal concepts. Alive claims that this code has UB due to use of gep inbounds: https://alive2.llvm.org/ce/z/zTctIR At the same time, the gep inbounds itself is not poison: https://alive2.llvm.org/ce/z/wxGGyu That makes it looks like Alive also constrains provenance based on gep inbounds, not just the value of the pointer.

In D93820#2471778, @nikic wrote:

In D93820#2471737, @nlopes wrote:

I only looked at the tests and they were correct before, see here: https://alive2.llvm.org/ce/z/UzW3pv
The tests are weird because they have 'gep inbounds'. The reason they are correct (and weird) is that the only way p - (int)p/sizeof(*p) is inbounds is p being null. Anything else will overflow.

This doesn't look right to me, at least not given current LangRef wording. Lets say we have gep inbounds p, -p, where p = ptr(base_addr = 1, offset = -1). This means that the address value of p is 0, but it has provenance of the object at base_addr = 1. As such, the inbounds is not violated (both p and the gep results are inbounds of the zero address), but we still change provenance.

LGTM

I only looked at the tests and they were correct before, see here: https://alive2.llvm.org/ce/z/UzW3pv
The tests are weird because they have 'gep inbounds'. The reason they are correct (and weird) is that the only way p - (int)p/sizeof(*p) is inbounds is p being null. Anything else will overflow.

This patch regressed the following test of Transforms/InstCombine/abs-1.ll:
(need to drop NSW in this case).

define i8 @nabs_canonical_3(i8 %x) {
%0:
  %cmp = icmp slt i8 %x, 0
  %neg = sub nsw i8 0, %x
  %abs = select i1 %cmp, i8 %x, i8 %neg
  ret i8 %abs
}
=>
define i8 @nabs_canonical_3(i8 %x) {
%0:
  %1 = abs i8 %x, 1
  %abs = sub nsw i8 0, %1
  ret i8 %abs
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

In D93065#2465056, @congzhe wrote:

In D93065#2463069, @nlopes wrote:

In practice, probably not a lot. But it may have implications for loop optimization, like:

for (i=0; some_bool && i < limit; ++i) {
...
}

If you remove the poison from the i+1 < limit bit it may make the work of SCEV harder (or impossible; didn't think the example through carefully).

Can I just make sure my understanding is correct -- so when we check the SCEV of some_bool && i < limit; we do recursion backwards on this select instruction (after this patch) or on an AND instruction (before this patch). If we choose the freeze approach, we'll do recursion on the AND instruction and eventually hit a freeze instruction which SCEV does not know how to handle, hence SCEV will just return CouldNotCompute?

As Ralf mentioned, the ship has sailed. Alloca and lifetime intrinsics were implemented like this several years ago. They were a quick hack to save stack space after inlining. That's it, and their design reflects the goals at the time.
We simply want to document what is implemented. @jdoerfert you seem to want to change the implementation and/or the design, which is a separate discussion. I suggest we first document how LLVM works and then if you want to make changes you start a *separate* discussion on the things you want to change, why, and what's the upgrade path, etc. We can't change the semantics of either alloca or lifetime intrinsics without an automatic upgrade path as otherwise we would break all frontends out there.

In D93065#2461517, @congzhe wrote:

In D93065#2461196, @nlopes wrote:

Using freeze loses information (if some of the inputs was poison). Plus It requires an extra op.
If we canonicalize around select there's no loss of information and it's just 1 instruction.

The disadvantage is that then we have 2 ways or doing boolean ANDs/ORs. Though most analyses can be patched easily, as most LLVM analyses' results are of the form "x has property foo unless it's poison". So for those analyses using and/or or select is the same (as the only difference between these is propagation of poison).
Other analyses/optimization can learn about select as needed.

Thank you for raising up the good point! I understand that we lose information by preventing poison values from propagation using freeze. But I'm unclear what would be the side effect or problem with that? I'd appreciate it if you could clarify a bit, thanks!

In D93376#2461218, @jdoerfert wrote:

What is the reason to restrict it to allocas? Just that we don't emit it right now? I don't see how that makes it conceptually better.

Using freeze loses information (if some of the inputs was poison). Plus It requires an extra op.
If we canonicalize around select there's no loss of information and it's just 1 instruction.

In D78938#2459973, @RKSimon wrote:

@BRevzin @nlopes This is causing MSVC build failure please can you take a look?

E:\llvm\llvm-project\llvm\include\llvm/DebugInfo/DWARF/DWARFDie.h(405): note: see declaration of 'std::reverse_iterator<llvm::DWARFDie::iterator>'
E:\llvm\llvm-project\llvm\lib\DWARFLinker\DWARFLinker.cpp(383): note: see reference to function template instantiation 'bool std::operator !=<llvm::DWARFDie::iterator,llvm::DWARFDie::iterator>(const std::reverse_iterator<llvm::DWARFDie::iterator> &,const std::reverse_iterator<llvm::DWARFDie::iterator> &)' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(2086): error C2039: '_Get_current': is not a member of 'std::reverse_iterator<llvm::DWARFDie::iterator>'
E:\llvm\llvm-project\llvm\include\llvm/DebugInfo/DWARF/DWARFDie.h(405): note: see declaration of 'std::reverse_iterator<llvm::DWARFDie::iterator>'

A contradiction in the proposed semantics vs the comments in the code is that we state that subsequent lifetime.start don't change the address of the alloca. There's only one address per alloca, even if it may have multiple disjoint liveness ranges.
It would be good to get confirmation from some CodeGen folks that the implemented algorithm respects this condition and that there are no plans to make the algorithm more aggressive in a way that may break this assumption.
Having multiple start/end pairs for the same alloca is not common, so I think imposing this condition is fine. It gives us free movement of gep/ptr2int, which is a good tradeoff.

In D93229#2452695, @lebedev.ri wrote:

/builddirs/llvm-project/build-Clang11-unknown$ /builddirs/llvm-project/build-Clang11-unknown/bin/opt -load /repositories/alive2/build-Clang-release/tv/tv.so -tv -vector-combine -mtriple=x86_64-- -mattr=avx2 -tv -o /dev/null --tv-smt-to=60000 /tmp/D93229.ll 

----------------------------------------
define <8 x i16> @t(* dereferenceable(128) align(128) %base) {
%0:
  %ptr = gep inbounds * dereferenceable(128) align(128) %base, 1 x i64 1
  %p = bitcast * %ptr to *
  %gep = gep inbounds * %p, 16 x i64 0, 2 x i64 1
  %s = load i16, * %gep, align 1
  %r = insertelement <8 x i16> undef, i16 %s, i64 0
  ret <8 x i16> %r
}
=>
define <8 x i16> @t(* dereferenceable(128) align(128) %base) {
%0:
  %ptr = gep inbounds * dereferenceable(128) align(128) %base, 1 x i64 1
  %p = bitcast * %ptr to *
  %gep = gep inbounds * %p, 16 x i64 0, 2 x i64 1
  %1 = bitcast * %gep to *
  %r = load <8 x i16>, * %1, align 1
  ret <8 x i16> %r
}
Transformation doesn't verify!

Thanks @lebedev.ri for the pointer!
I started working on exactly the same thing as I was trying to link a C++20 project with LLVM.
@BRevzin is there anything missing in this patch? Do you have commit access or do you need help to land this?

In D90529#2449703, @nlopes wrote:

For the partial undef memory access example that Juneyoung gave.. Well, maybe we need to make it UB to dereference a non-deterministic value. Doesn't seem like it's a very useful thing to do, and this non-determinism comes from some previous undefined behavior, so it seems fine to just make dereference of partial undef UB. Simplifies things.

There was a discussion for this: https://groups.google.com/g/llvm-dev/c/2Qk4fOHUoAE/m/OxZa3bIhAgAJ
This partially undef thing is a bit painful.. :/

I like where this is going. Most of LLVM's alias analysis produce information that only holds if the value is not poison. Since these attributes are derived from said analysis, then it makes sense then they have the same "X is poison or foo(X) holds" semantics.
I agree that certain attributes are different, like dereferenceable. It is useless if the value might be poison as well. Though we may go with the same semantics and then require the noundef attribute to make it useful. Seems like a good way to go as well.