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.

LGTM!
Thanks a lot for working on this!

Nice!
BTW, another popular idiom is to store data in the last few bits of the pointer (e.g., LLVM's own PointerIntPair). I guess that one can also be implement by casting the ptr to char* and doing operations over that.

In D90382#2375905, @nikic wrote:

@nlopes I think we should adjust the funnel shift definition to say that it blocks poison on one operand if the shift amount is zero. Basically the poison semantics should be "as if" the funnel shift were expanded, which does include an explicit select for the zero shift amount case.

Alive2 says this test is incorrect (because select blocks poison and funnel shift doesn't):

define i8 @fshr_select(i8 %x, i8 %y, i8 %shamt) {
%0:
  %cmp = icmp eq i8 %shamt, 0
  %sub = sub i8 8, %shamt
  %shr = lshr i8 %y, %shamt
  %shl = shl i8 %x, %sub
  %or = or i8 %shl, %shr
  %r = select i1 %cmp, i8 %y, i8 %or
  ret i8 %r
}
=>
define i8 @fshr_select(i8 %x, i8 %y, i8 %shamt) {
%0:
  %r = fshr i8 %x, i8 %y, i8 %shamt
  ret i8 %r
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

LGTM modulo the two comments. Thanks for writing this down!

In D90637#2369842, @nikic wrote:

Hm, I think we need to clarify this in LangRef. We definitely assume this interpretation (unsigned base and signed offset) in some places (e.g. https://github.com/llvm/llvm-project/blob/c938b4a1ed43f3075155e16a7c2792ca8c122258/llvm/lib/Analysis/ScalarEvolution.cpp#L5061-L5072 and I'm pretty sure I've seen it elsewhere as well), but LangRef is really not clear on this point. It's also not completely obvious where the assumption that the pointer address space is unsigned comes from. E.g. on x86-64 the canonical address space is signed (but I don't know about other architectures). We need to clarify whether having an allocated object at [0xffffffff, 0x00000001] is legal (signed address space), [0x7fffffff, 0x80000001] is legal (unsigned address space) or both.

FWIW, here's a related bug (fixed already): https://bugs.llvm.org/show_bug.cgi?id=42699

Alive2 complains about one of the test cases:

define i64 @fshr_sub_mask(i64 %x, i64 %y, i64 %a) {
  %mask = and i64 %a, 63
  %shr = lshr i64 %x, %mask
  %sub = sub nsw nuw i64 64, %mask
  %shl = shl i64 %y, %sub
  %r = or i64 %shl, %shr
  ret i64 %r
}
=>
define i64 @fshr_sub_mask(i64 %x, i64 %y, i64 %a) {
  %r = fshr i64 %x, i64 %y, i64 %a
  ret i64 %r
}
Transformation doesn't verify!
ERROR: Value mismatch

In D88979#2323940, @lebedev.ri wrote:

In D88979#2323935, @nikic wrote:

LGTM

@nlopes does this look good to you?

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

I think i don't agree with you there.
I agree with @nlopes, the end goal will be to basically disallow fusing of inttoptr/ptrtoint into loads,
disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
And all that eventually boils down to updating CastInst::isNoopCast()/CastInst::isEliminableCastPair().

In D88788#2313981, @lebedev.ri wrote:

In D88788#2313958, @nlopes wrote:

In D88788#2311004, @efriedma wrote:

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

Agreed. But until LLVM removes pointer sub-types it's convenient to get the alloca type right to avoid bitcast on every access anyway.
When pointer sub-types goes away, I guess all this code in SROA to find the right type for alloca would go away, but as you say it would have to be replaced with code to get the right load/store type instead. (FWIW Alive2's alloca only takes the number of bytes to allocate as argument)
So I see this patch as a step in the right direction.

So i don't do this blindly to find out that is'a bad idea, can we agree on the baseline here?
How should this be done properly? Instead of relying on the allocation type, would the D88842's approach be applicable here?

ptrtoint and inttoptr are different beasts.
Supporting ptrtoint is much simpler. It's inttoptr that makes me uncomfortable. I don't known enough about SCEV to know how it handles these unknown nodes.
To me, the patch makes sense for ptrtoint (as you say, it's just a zext/sext of some unknown value; worst case it's poison if we want to be strict about OOB). Though I can't comment on the inttoptr case (some SCEV expert needs to chime in).

In D88788#2311004, @efriedma wrote:

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

My meta-comment about this patch is that I'm not sure LangRef is the right place for this content. I see LangRef as the stuff that is set in stone, not necessarily for ongoing discussions.
However, since LangRef doesn't get these bits right, it might be ok to have a warning section about stuff that is disputed/under discussion so that readers know that part is not set in stone.

I don't like the direction of this patch because it will remove inttoptr instructions that were present in the original program. In the same way that optimizations shouldn't introduce inttoptr, they shouldn't fuse them with loads either.
If the original program had the inttoptr cast and you fold it with a load instruction, then you are asking the load to do the cast. This sort of type punning is not ok. I strongly disagree with Chandler that LLVM's memory is not typed. It needs to distinguish between integers and pointers, otherwise I don't know how to make LLVM correct without a significant perf penalty.
If LLVM's memory was untyped, we would have to assume that every pointer load could be doing an implicit inttoptr cast, which would havoc most optimizations. The fact that some optimizations, like sroa, assume that the memory is untyped and most of the others don't is a source of miscompilations.

Love it, thanks!
This gets rid of a lot of type punning issues through load/store of integers. Not introducing inttoptr during optimization is a very healthy goal.

Ok, let me make it more concrete.
it seems we have 3 possible semantics:

1. volatile accesses never trap, but rather trigger UB when the address is not dereferenceable
2. they trap if the address is not dereferenceable
3. they may trap regardless (i.e., they can never be removed). Alternatively we can state that the load/store address traces are externally observable and can't change

I would say let's write an RFC and see if there are other opinions. Also, @nlopes what does alive2 think of such a proposal?

FYI: I've just run Alive2 (already patched for this new semantics) on the test suite and no regressions reported.

In D86815#2245481, @rsmith wrote:

In D86815#2245467, @arsenm wrote:

In D86815#2245466, @arsenm wrote:

Why would we change this? What's the point of having separate memcpy and memmove intrinsics?

I'm reading this as clang mis-uses llvm.memcpy when it probably should be using llvm.memmove

There is a longstanding assumption made by ~every compiler that memcpy(p, p, n) is safe. That's what we should be encoding here. We should not be removing all overlap restrictions.

The reasoning looks good, though the case when offset + size overflows doesn't seem to be considered.

FYI InstSimplify doing distribution of undef is a known bug: https://bugs.llvm.org/show_bug.cgi?id=33165

In D83440#2143450, @brooksmoses wrote:

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

In D83360#2142457, @efriedma wrote:

that's fine but I still don't understand why the counterexample to my version says %x2 in @src can be undef

If I'm understanding correctly, this reduces to something like the following:

define i32 @src() {

%x2 = freeze i32 undef
ret i32 %x2

}

define i32 @tgt() {

ret i32 undef

}

This seems a little suspect, yes.

In D83360#2140241, @craig.topper wrote:

Alive does like this https://alive2.llvm.org/ce/z/yhibbe which is what I was going to implement.

Here's an end-to-end miscompilation: https://bugs.llvm.org/show_bug.cgi?id=31633

I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away):

int f() { return INT_MAX + 1; }

The passes we are aware that introduce branch on poison are: IndVarSimplify, LoopUnswitch, SimpleLoopUnswitch, and SimpifyCFG.
(https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=4beb2b117e2fdd2c)

In D78631#1996721, @spatel wrote:

Side note - Alive2 does not seem to recognize the keyword 'fast' currently (or we would have seen more tests flagged as incorrect).

In D77868#1977808, @spatel wrote:

// select Cond, T, false --> Cond & T
if (match(F, m_ZeroInt()))
  return SimplifyAndInst(Cond, T, Q);

I still do not see how this can go wrong in practice. If InstSimplify can prove that T is poison, then doesn't it always manifest that knowledge by saying T is undef?
Looking at it another way (see if you can spot a logic flaw):

1. Assume T is poison.
2. Assume InstSimplify fails to simplify T to undef or constant.
3. For poison T' (either T or some other poisoned value) to leak through as the result, InstSimplify must prove that Cond & T' == T'.
4. So InstSimplify must prove that Cond == true or Cond == T'.
5. If Cond == T', there's no problem (if Cond is poison, the select is poison).
6. Therefore -- for there to be a problem -- we must prove that Cond is true.
7. If we can prove that Cond is true, then that guarantees that value T' can't depend on Cond (nothing depends on Cond - it simplifies to constant true).

Yes, introducing branches on a variable that may be undef/poison is not legal. However, you can use freeze to make it safe.
I think @aqjune fixed loop unswitching already. (don't recall if that was the reverted patch). It's true there a couple more places left to fix.

In D76973#1948751, @spatel wrote:

In D76973#1948172, @fhahn wrote:

We should probably also think about what to do with all the tests that contain branches on undef. Also, bugpoint should probably stop creating branches on undef with this spelled out as is?

When I've updated other tests that would break with changed/improved undef semantics, I usually replace the 'undef' with a parameter like this:
rGfebcb24f1490
Maybe we can script that to fix most of the tests?

LGTM

LGTM (modulo the style fix).
assume poison is UB.

I agree with @aqjune that stating clearly the definition of object in this context.
See this example in the C spec:

For example, the second call of f in g has undefined behavior because each of d[1] through d[49] is accessed through both p and q.

LGTM overall, just some nitpicks.
Something I would add is a link to the list of attributes (is this function-attributes?).

I'm not familiar with the code of this pass, but is there a cheap way of identifying that the two operations are in the same basic block?
If so, you could take the intersection of the aliasing information rather than the union. Because if both ops are guaranteed to execute then the tightest aliasing still has to hold.
(being in the same BB doesn't imply that both instructions are executed, but there's code in ValueTracking perhaps that can check that)

The patch & semantics look good to me, but I'm not a backend expert. I'll leave the final LGTM to someone else.
It would be awesome if we could get this in for 10.0 so that we have complete support for freeze.

@lebedev.ri I agree with you that the semantics of these alignment builtins should only return a pointer that is of the same object as the one given as input.
Otherwise, these builtins would be even worst that ptr2int/int2ptr, since their result could alias with any other pointer in the program, not just the escaped pointers.

LGTM, thanks!

LGTM. The table you provided was very interesting to see, thanks!

In D70749#1767639, @jdoerfert wrote:

You said "Changing the semantics on non-respecting the tags from UB to poison doesn't help either.", could you elaborate why?
If there are no uses of a violating instantiation (undef/null passed to a non-null) we would not get UB but an unused poison value.

I think it's clear that dead arg elimination is incorrect in replacing a valid pointer with null in an attributed with non-null tag. Changing the semantics on non-respecting the tags from UB to poison doesn't help either.
The problem with dropping attributes is that a given function call site, the attributes to be considered are the union of the attributes in the function call and in the callee declaration. We can't rely drop attributes from the callee since some linking later may add them back.

LGTM
Reference to the discussion: http://lists.llvm.org/pipermail/llvm-dev/2019-November/137243.html