[Attributor] AAUndefinedBehavior: Use AAValueSimplify in memory accessing instructions.

[baziotis]

Summary

Query AAValueSimplify on pointers in memory accessing instructions to take advantage of the constant propagation (or any other value simplification) of such values.

[uenoku]

There's one test case that is worrying: IPConstantProp/PR26044.ll. With this patch, a from undef becomes load from null and I don't see the reason.

I think this is no problem. It is because of on-demand creation. It is the first time to create AAValueSimplify to a pointer operand. The deduction is like this:

1. The assumption which claims "for.cond1 is dead" is rejected because AAIsDead doesn't know there is UB.
2. %e.2 is simplified to null because %e.2 can be null or undef.
3. %e.2 is replaced with null in manifest

So once AAIsDead can get to interact with AAUB, the problem will be solved.
Please add FIXME now.

[baziotis]

In D71960#1797942, @uenoku wrote:

There's one test case that is worrying: IPConstantProp/PR26044.ll. With this patch, a from undef becomes load from null and I don't see the reason.

I think this is no problem. It is because of on-demand creation. It is the first time to create AAValueSimplify to a pointer operand. The deduction is like this:

1. The assumption which claims "for.cond1 is dead" is rejected because AAIsDead doesn't know there is UB.
2. %e.2 is simplified to null because %e.2 can be null or undef.
3. %e.2 is replaced with null in manifest

So once AAIsDead can get to interact with AAUB, the problem will be solved.
Please add FIXME now.

Hmm, actually by looking again the code, I think that null is the correct value.
From looking at the IR, without thinking what Attributor does, it seems that the only valid value is null (because it comes from entry while the others are
unreachable).
Now, the more interesting part is actually looking what the Attributor does. If I followed the code correctly, previously we didn't query AAValueSimplify.
What happens if we do (and correct me if I'm wrong, it seems useful to me to know how it works) is that %e.2 is a floating value,
which calls genericValueTraversal(), which for the phi node, calls VisitValueCB with the 3 incoming values: null, undef and undef.
This in turn calls checkAndUpdate() which ignores UndefValue and finalizes on null (but if we had other different values, this:

if (AccumulatedSimplifiedValue.hasValue())
  return AccumulatedSimplifiedValue == QueryingValueSimplified;

in checkAndUpdate() would return false because of conflicting values, effectively indicating pessimistic fixpoint).

Now, other than that, I guess that with this:

So once AAIsDead can get to interact with AAUB, the problem will be solved.

you meant that when AAIsDead will interact with AAUB, the former will ignore the 2 phis in genericValueTraversal() since it will consider them dead.

[uenoku]

In D71960#1797992, @baziotis wrote:

Now, the more interesting part is actually looking what the Attributor does. If I followed the code correctly, previously we didn't query AAValueSimplify.
What happens if we do (and correct me if I'm wrong, it seems useful to me to know how it works) is that %e.2 is a floating value,
which calls genericValueTraversal(), which for the phi node, calls VisitValueCB with the 3 incoming values: null, undef and undef.
This in turn calls checkAndUpdate() which ignores UndefValue and finalizes on null (but if we had other different values, this:

if (AccumulatedSimplifiedValue.hasValue())
  return AccumulatedSimplifiedValue == QueryingValueSimplified;

in checkAndUpdate() would return false because of conflicting values, effectively indicating pessimistic fixpoint).

Now, other than that, I guess that with this:

So once AAIsDead can get to interact with AAUB, the problem will be solved.

you meant that when AAIsDead will interact with AAUB, the former will ignore the 2 phis in genericValueTraversal() since it will consider them dead.

Yes, exactly. Btw, my understating is that load inst from undef pointer causes UB even if "null-pointer-is-valid"="true" (https://llvm.org/docs/LangRef.html#pointeraliasing).
Then, I think @fn_no_null_opt can be optimized aggressively, is this correct? If so, please add FIXME.

[baziotis]

In D71960#1798010, @uenoku wrote:

In D71960#1797992, @baziotis wrote:

Now, the more interesting part is actually looking what the Attributor does. If I followed the code correctly, previously we didn't query AAValueSimplify.
What happens if we do (and correct me if I'm wrong, it seems useful to me to know how it works) is that %e.2 is a floating value,
which calls genericValueTraversal(), which for the phi node, calls VisitValueCB with the 3 incoming values: null, undef and undef.
This in turn calls checkAndUpdate() which ignores UndefValue and finalizes on null (but if we had other different values, this:

if (AccumulatedSimplifiedValue.hasValue())
  return AccumulatedSimplifiedValue == QueryingValueSimplified;

in checkAndUpdate() would return false because of conflicting values, effectively indicating pessimistic fixpoint).

Now, other than that, I guess that with this:

So once AAIsDead can get to interact with AAUB, the problem will be solved.

you meant that when AAIsDead will interact with AAUB, the former will ignore the 2 phis in genericValueTraversal() since it will consider them dead.

Yes, exactly.

Great :)

Btw, my understating is that load inst from undef pointer causes UB even if "null-pointer-is-valid"="true" (https://llvm.org/docs/LangRef.html#pointeraliasing).
Then, I think @fn_no_null_opt can be optimized aggressively, is this correct? If so, please add FIXME.

Oh btw, I missed the code haha:

Hmm, actually by looking again the code, I think that null is the correct value.

It so isn't the correct value :P I thought the code was: phi = [entry, null], [for.cond1, undef], [for.cond1, undef] but it is: phi = [entry, undef], [for.cond1, null], [for.cond1, null]. That's why you mentioned the FIXME above. I'll add it.
And yes, what you mentioned is totally UB and the code is written in such a way as to handle it. But it doesn't, I'll add a FIXME for that as well.
It seems there's some problem in general with the propagation of values. For example, in undefined_behavior.ll test, if you check load_null_propagated() the null is propagated, but AAUB doesn't pick it up.

[baziotis]

• Added FIXMEs.

Edit: Ignore that, I upload the previous diff by mistake.

[baziotis]

Actually added FIXMEs.

[jdoerfert]

llvm/lib/Transforms/IPO/Attributor.cpp

2360

Where is stopOnUndefOrAssumed defined?

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

9

This test is actually really broken. The function can never be executed w/o causing UB. We need to fix these tests to have some behavior.

[baziotis]

llvm/lib/Transforms/IPO/Attributor.cpp

2360

Sorry, I should have mentioned in the summary that this diff is based on: https://reviews.llvm.org/D71799
That is already accepted and will be committed so I thought it would be good to not wait for it to actually be commited.

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

9

Did I break anything? As far as I could understand, this was true before as well.
Or just a general comment? I personally think that now we have AAUB, it's ok to have "always UB" functions on tests that are supposed to test UB (like in the undefined_behavior.ll) but here it may be irrelevant to always have UB.

[jdoerfert]

(Don't wait for me to review/accept these even if I leave random comments once in a while.)

llvm/lib/Transforms/IPO/Attributor.cpp

2360

No worries, I just missed that part of D71799.

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

We should also repair this test. pass a valid pointer into callee, e.g., an argument.

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

9

The test was broken, the new AAUB just exposed it (more and more). Since this is supposed to test AAValueSimplify, we should remove the UB. Can you replace the undef in the branch with some argument value (which you need to add), and the undef in the phi with %p. Then let the jump go to some exit block that returns. At least that way we have defined behavior until the Attributor gets smart enough to realize the endless loop has no atomic side effect and can be removed as UB as well ;)

[baziotis]

In D71960#1798226, @jdoerfert wrote:

(Don't wait for me to review/accept these even if I leave random comments once in a while.)

Ok, I'll wait for Hideto and Stefan, thanks for the comments though.

[baziotis]

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

Ok,yes. Btw, the nonnull and dereferenceable attributes for %A are wrong right? I guess these are the kind of checks for violations you have proposed that AAUB could do.

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

9

If I understood correctly, the code will look like this:

entry:
  br label %if.end

for.cond1:
  br i1 %C, label %if.end, label %if.end

if.end:
  %e.2 = phi i32* [ %P, %entry ], [ null, %for.cond1 ], [ null, %for.cond1 ]
  %0 = load i32, i32* %e.2, align 4
  ...
  br label %exit
exit:
  ret void
}

where %C is an argument. If we do that, there's no endless loop and the %for.cond1 branch is unreachable (not because AAUB, but because there's no way to reach it).

[uenoku]

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

Btw, the nonnull and dereferenceable attributes for %A are wrong right?

I think to mark nonnull and dereferenceable attributes for %A itself is not wrong.
(but anyway %A will be deleted with D68765 or additional patches)
I mean passing null to this function is UB too. So AAUB can prove

%X = call i32 @callee(i1 false, i32* null)

is UB.

[baziotis]

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

So, generally for attributes, I'm not sure when is the caller responsible for it to hold or the "marker" (or both).
What I mean is that the way I see it, in this example, there are 2 cases (in the general case, replace "parameter" with "value that can be attributed"):

1. If the caller is responsible, then I can mark any attributes in the parameter (e.g. nonnull) and whoever calls this function is responsible for the attributes to hold (e.g. not pass a null). Otherwise, it is UB.
2. If the marker is responsible, then whoever marks a parameter (e.g. nonnull) is responsible to validate that anyone who

calls this function does not pass a null parameter.

The 1) makes more sense since 2) is practically impossible if the function is external. But 2) isn't really what the Attributor does?
(e.g. the Attributor tries to be sure that an argument is dereferenceable to mark it as such).

[uenoku]

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

If the caller is responsible, then I can mark any attributes in the parameter (e.g. nonnull) and whoever calls this function is responsible for the attributes to hold (e.g. not pass a null). Otherwise, it is UB.
If the marker is responsible, then whoever marks a parameter (e.g. nonnull) is responsible to validate that anyone who calls this function does not pass a null parameter.
The 1) makes more sense since 2) is practically impossible if the function is external. But 2) isn't really what the Attributor does?

Yes, almost all the attributes are using only 2) but some (ex. nonnull, align, dereferenceable) are doing deduction in both ways. In this case, %A is known as nonnull because %A is loaded at an entry point.

If the function is an external function, Attributor gives up 2). (please look at checkForAllCallsites implementation)

[baziotis]

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

10–11

Aha, got it, thanks!

[baziotis]

• Updated tests

Note that I removed the second test in IPConstantProp/PR26044.ll since as far as I could tell, it is based on having null in a load.
Since this test tests value simplification and not UB (which is the reason why the first test was changed as well), probably this would not
be very useful.

[baziotis]

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

9

Then let the jump go to some exit block that returns

I had forgotten about this. Sorry, just yesterday I learned about the terminology of loops. So I didn't get that "exit block" is a characteristic term that makes clear that you were referring to the "else" jump on the conditional branch and not the jump on the latch.
It should be ok now.

[jdoerfert]

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

88

Please do not delete the tests but "repair" them if possible. Replace undef above with argument values such that we could still propagate the load null, which is valid in @fn_no_null_opt, into @fn0.

[baziotis]

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

88

Ah right. Let me make the "null-pointer-is-valid"="true" more obvious because I keep forgetting it.

[baziotis]

• Repaired test case.

[baziotis]

Anything else needed for that?

[jdoerfert]

LGTM, two things before we commit though:

1. update the commit message
2. add a test that checks the functionality. the test changes in this patch "repair" existing tests to not expose unwanted UB but we want to have at least one test that exhibits UB and which gets exploited due to this patch.

[baziotis]

• Update commit message and test cases

in ArgumentPromotion/2008-07-02-array-indexing.ll, there was a weird align value that was only in the CHECK directives so it didn't seem it was coming from somewhere. I had to change it to make it pass. Please verify that it is not needed.

[baziotis]

In D71960#1870868, @jdoerfert wrote:

2. add a test that checks the functionality. the test changes in this patch "repair" existing tests to not expose unwanted UB but we want to have at least one test that exhibits UB and which gets exploited due to this patch.

Something more complicated than load_null_propagated() and / or cond_br_on_undef* functions (in undefined_behavior.ll testcase) ?

[jdoerfert]

In D71960#1872563, @baziotis wrote:

• Update commit message and test cases

in ArgumentPromotion/2008-07-02-array-indexing.ll, there was a weird align value that was only in the CHECK directives so it didn't seem it was coming from somewhere. I had to change it to make it pass. Please verify that it is not needed.

The align was "derived" from a null pointer, which has every alignment. Since you made the UB go away by using an argument instead of loading from null, the alignment is no longer deduced. All as expected :)

In D71960#1872571, @baziotis wrote:

In D71960#1870868, @jdoerfert wrote:

2. add a test that checks the functionality. the test changes in this patch "repair" existing tests to not expose unwanted UB but we want to have at least one test that exhibits UB and which gets exploited due to this patch.

Something more complicated than load_null_propagated() and / or cond_br_on_undef* functions (in undefined_behavior.ll testcase) ?

Nothing complicated. Along those lines but as far as I can tell this patch does not include changes to these tests, correct?

[baziotis]

In D71960#1873204, @jdoerfert wrote:

In D71960#1872563, @baziotis wrote:

• Update commit message and test cases

in ArgumentPromotion/2008-07-02-array-indexing.ll, there was a weird align value that was only in the CHECK directives so it didn't seem it was coming from somewhere. I had to change it to make it pass. Please verify that it is not needed.

The align was "derived" from a null pointer, which has every alignment. Since you made the UB go away by using an argument instead of loading from null, the alignment is no longer deduced. All as expected :)

Ok and so it put a random value, got it :)

In D71960#1872571, @baziotis wrote:

In D71960#1870868, @jdoerfert wrote:

2. add a test that checks the functionality. the test changes in this patch "repair" existing tests to not expose unwanted UB but we want to have at least one test that exhibits UB and which gets exploited due to this patch.

Something more complicated than load_null_propagated() and / or cond_br_on_undef* functions (in undefined_behavior.ll testcase) ?

Nothing complicated. Along those lines but as far as I can tell this patch does not include changes to these tests, correct?

Correct. TBH, it's been some time since I saw this that for example even I forgot that load_null_propagated() was not introduced in this patch (although it still puzzles me how it worked before this patch).
I'll add more tests.

[baziotis]

• Added tests

[jdoerfert]

In D71960#1873401, @baziotis wrote:

• Added tests

All the tests have FIXMEs, right? Can you explain why they don't work yet? We also need tests that didn't work before but now, maybe just copies of the one you repaired but without the "repair" part.

[baziotis]

In D71960#1873456, @jdoerfert wrote:

In D71960#1873401, @baziotis wrote:

• Added tests

All the tests have FIXMEs, right? Can you explain why they don't work yet?

Yes. For most tests the reason is that AAValueSimplify doesn't give a known value. In other words, in stopOnUndefOrAssumed(), we don't get past the first if (if (!ValueSimplifyAA.isKnown())).

Seeing that again though, returning from this if effectively means that the instruction remains assumed UB.
The whole procedure ends with the instruction still assumed UB. So, doesn't that fixpoint to known UB actually ?

But then, check this:

define internal i32* @ret_null(i32* %A) {
  ret i32* %A
}

define void @load_null_propagated(i32* %A) {
  %ptr = call i32* @ret_null(i32* %A)
  %a = load i32, i32* %ptr
  ret void
}

The output is (same with AAUB disabled):

define void @load_null_propagated(i32* nocapture nofree readonly %A) #0 {
  %a = load i32, i32* undef
  ret void
}

First of all, even if what I said above is correct, AAValueSimplify still gives us no value for the pointer which keeps an actually non-UB instruction to assumed till the end.
But then second, isn't that output wrong in general ?

We also need tests that didn't work before but now, maybe just copies of the one you repaired but without the "repair" part.

Hmm, yes, that should be feasible to do in 2008-07-02-array-indexing.ll and PR26044.ll. Guessing what else this patch could "repair" / improve and making new tests is more challenging.

[jdoerfert]

In D71960#1875545, @baziotis wrote:

In D71960#1873456, @jdoerfert wrote:

In D71960#1873401, @baziotis wrote:

• Added tests

All the tests have FIXMEs, right? Can you explain why they don't work yet?

Yes. For most tests the reason is that AAValueSimplify doesn't give a known value. In other words, in stopOnUndefOrAssumed(), we don't get past the first if (if (!ValueSimplifyAA.isKnown())).

Seeing that again though, returning from this if effectively means that the instruction remains assumed UB.
The whole procedure ends with the instruction still assumed UB. So, doesn't that fixpoint to known UB actually ?

But then, check this:

define internal i32* @ret_null(i32* %A) {
  ret i32* %A
}

define void @load_null_propagated(i32* %A) {
  %ptr = call i32* @ret_null(i32* %A)
  %a = load i32, i32* %ptr
  ret void
}

The output is (same with AAUB disabled):

define void @load_null_propagated(i32* nocapture nofree readonly %A) #0 {
  %a = load i32, i32* undef
  ret void
}

First of all, even if what I said above is correct, AAValueSimplify still gives us no value for the pointer which keeps an actually non-UB instruction to assumed till the end.
But then second, isn't that output wrong in general ?

I think this is a bad artifact of our current "code deletion" at the end of the Attributor run. Make the load live and it will not happen, I mean, the load in the example above can be removed either way.

define internal i32* @ret_null(i32* %A) {
   ret i32* %A
 }
 
 define void @load_null_propagated(i32* %A) {
   %ptr = call i32* @ret_null(i32* %A)
   %a = load i32, i32* %ptr
   ret void
 }

I wanted to confirm and I noticed that in-tree we actually do not replace with undef, did you rebase your patch recently?

We also need tests that didn't work before but now, maybe just copies of the one you repaired but without the "repair" part.

Hmm, yes, that should be feasible to do in 2008-07-02-array-indexing.ll and PR26044.ll. Guessing what else this patch could "repair" / improve and making new tests is more challenging.

Assuming this patch has some effect, we need to have a test for it. Then we merge and make bigger changes.

[baziotis]

In D71960#1875557, @jdoerfert wrote:

In D71960#1875545, @baziotis wrote:

In D71960#1873456, @jdoerfert wrote:

In D71960#1873401, @baziotis wrote:

• Added tests

All the tests have FIXMEs, right? Can you explain why they don't work yet?

Yes. For most tests the reason is that AAValueSimplify doesn't give a known value. In other words, in stopOnUndefOrAssumed(), we don't get past the first if (if (!ValueSimplifyAA.isKnown())).

Seeing that again though, returning from this if effectively means that the instruction remains assumed UB.
The whole procedure ends with the instruction still assumed UB. So, doesn't that fixpoint to known UB actually ?

But then, check this:

define internal i32* @ret_null(i32* %A) {
  ret i32* %A
}

define void @load_null_propagated(i32* %A) {
  %ptr = call i32* @ret_null(i32* %A)
  %a = load i32, i32* %ptr
  ret void
}

The output is (same with AAUB disabled):

define void @load_null_propagated(i32* nocapture nofree readonly %A) #0 {
  %a = load i32, i32* undef
  ret void
}

First of all, even if what I said above is correct, AAValueSimplify still gives us no value for the pointer which keeps an actually non-UB instruction to assumed till the end.
But then second, isn't that output wrong in general ?

I think this is a bad artifact of our current "code deletion" at the end of the Attributor run. Make the load live and it will not happen, I mean, the load in the example above can be removed either way.

You mean because it's unused anyway? Indeed if we return %a for example, it's not replaced with undef.

define internal i32* @ret_null(i32* %A) {
   ret i32* %A
 }
 
 define void @load_null_propagated(i32* %A) {
   %ptr = call i32* @ret_null(i32* %A)
   %a = load i32, i32* %ptr
   ret void
 }

I wanted to confirm and I noticed that in-tree we actually do not replace with undef, did you rebase your patch recently?

Not too long ago, about 3-4 days ago but now that I rebased again, it seems it acts differently. I'll update the diff.

Edit: And btw sorry for not following the Attributor closely. Time is limited right now,
I hope I'll have more time in the future.

We also need tests that didn't work before but now, maybe just copies of the one you repaired but without the "repair" part.

Hmm, yes, that should be feasible to do in 2008-07-02-array-indexing.ll and PR26044.ll. Guessing what else this patch could "repair" / improve and making new tests is more challenging.

Assuming this patch has some effect, we need to have a test for it. Then we merge and make bigger changes.

Yes, I think new tests test the new things. I'll update the repaired as well and then we see if it needs anything more.

[baziotis]

• Rebased

Now tests are a lot better!

(I will follow with copies of repaired tests)

[xbolva00]

I have a similar case how to exploit UB to simplify code:

int process(char *p) __attribute__((nonnull));


void doit(bool b, char *d) {
    char *p = 0;
    if (b)
        p = d;
    process(p);
}

https://godbolt.org/z/eYcApm

In this case, it is invalid to call process with null pointer since args are nonnull, so we can remove branch and call process(d) directly.

[baziotis]

In D71960#1877001, @xbolva00 wrote:

I have a similar case how to exploit UB to simplify code:

int process(char *p) __attribute__((nonnull));


void doit(bool b, char *d) {
    char *p = 0;
    if (b)
        p = d;
    process(p);
}

https://godbolt.org/z/eYcApm

In this case, it is invalid to call process with null pointer since args are nonnull, so we can remove branch and call process(d) directly.

Thanks for the proposal! At first glance, deleting the branch is not straightforward and may not make a lot of sense. Reaching the call, you have a phi node basically saying:

• If you got into the branch, the value is d.
• If not, the value is p (which with constant propagation, let's say null).

Deleting the branch first of all seems to require somehow going "backwards", tracking where this (or these) value came from. Second, I can't find a reasoning on how to generalize this (except since we have UB, do whatever)
because the previous code is not invalid, only the call.

That said, personally I think it's a very good idea to check if null arguments are passed to nonnull parameters and consider that UB. Then, we can make the call unreachable.

[xbolva00]

That said, personally I think it's a very good idea to check if null arguments are passed to nonnull parameters and consider that UB. Then, we can make the call unreachable.

Yeah.

[jdoerfert]

One last nit with one of the tests, then I'll commit this.

In D71960#1877265, @baziotis wrote:

In D71960#1877001, @xbolva00 wrote:

I have a similar case how to exploit UB to simplify code:

int process(char *p) __attribute__((nonnull));


void doit(bool b, char *d) {
    char *p = 0;
    if (b)
        p = d;
    process(p);
}

https://godbolt.org/z/eYcApm

In this case, it is invalid to call process with null pointer since args are nonnull, so we can remove branch and call process(d) directly.

Nit: In the godbold example you don't even need the __attribute__((nonnull)) for this, the access is sufficient ;)

Thanks for the proposal! At first glance, deleting the branch is not straightforward and may not make a lot of sense. Reaching the call, you have a phi node basically saying:

• If you got into the branch, the value is d.
• If not, the value is p (which with constant propagation, let's say null).

This is where AAIsDead needs to talk to AAUnreachable at some point but we have simpler cases to tackle first.

Deleting the branch first of all seems to require somehow going "backwards", tracking where this (or these) value came from. Second, I can't find a reasoning on how to generalize this (except since we have UB, do whatever)
because the previous code is not invalid, only the call.

I was always hoping for backwards propagation of UB. Let's start with propagating unreachable backwards. There is already a TODO in the code btw:
// TODO: look for (assumed) UB to backwards propagate "deadness".

A good first step would be to check the MustBeExecutedContext either backwards from known UB, e.g. hitting an unreachable, or forward after a control dependence change (e.g, after a conditional branch).
The backwards propagation is not cleaned up and integrated yet (for MustBeExecutedContext, it's somewhere in a review though).
One could maybe work on that or go with the forward approach. This should be done in AAIsDeadFunction.

That said, personally I think it's a very good idea to check if null arguments are passed to nonnull parameters and consider that UB. Then, we can make the call unreachable.

I generally agree but we have to be a bit careful. People don't like "obviously harmless" UB to be exploited, e.g., passing null to a nonnull argument that is unused should not cause an unreachable

llvm/test/Transforms/Attributor/undefined_behavior.ll

80

You have to return the value or sth. As it is, the call has no side-effects and is deleted either way.

[baziotis]

In D71960#1877393, @jdoerfert wrote:

One last nit with one of the tests, then I'll commit this.

Ok thanks, I'll address it. Looking at the repaired tests, I can't see something that this patch exposed
that is not tested in undefined_behavior.ll test.

In D71960#1877265, @baziotis wrote:

In D71960#1877001, @xbolva00 wrote:

I have a similar case how to exploit UB to simplify code:

int process(char *p) __attribute__((nonnull));


void doit(bool b, char *d) {
    char *p = 0;
    if (b)
        p = d;
    process(p);
}

https://godbolt.org/z/eYcApm

In this case, it is invalid to call process with null pointer since args are nonnull, so we can remove branch and call process(d) directly.

Nit: In the godbold example you don't even need the __attribute__((nonnull)) for this, the access is sufficient ;)

May I add: You don't need it because of the Attributor (which puts the nonnull since there is an access).

Thanks for the proposal! At first glance, deleting the branch is not straightforward and may not make a lot of sense. Reaching the call, you have a phi node basically saying:

• If you got into the branch, the value is d.
• If not, the value is p (which with constant propagation, let's say null).

This is where AAIsDead needs to talk to AAUnreachable at some point but we have simpler cases to tackle first.

Deleting the branch first of all seems to require somehow going "backwards", tracking where this (or these) value came from. Second, I can't find a reasoning on how to generalize this (except since we have UB, do whatever)
because the previous code is not invalid, only the call.

I was always hoping for backwards propagation of UB. Let's start with propagating unreachable backwards. There is already a TODO in the code btw:
// TODO: look for (assumed) UB to backwards propagate "deadness".

A good first step would be to check the MustBeExecutedContext either backwards from known UB, e.g. hitting an unreachable, or forward after a control dependence change (e.g, after a conditional branch).
The backwards propagation is not cleaned up and integrated yet (for MustBeExecutedContext, it's somewhere in a review though).
One could maybe work on that or go with the forward approach. This should be done in AAIsDeadFunction.

It seems that what you're describing is similar to: https://reviews.llvm.org/D71974 when checking for known UB.

That said, personally I think it's a very good idea to check if null arguments are passed to nonnull parameters and consider that UB. Then, we can make the call unreachable.

I generally agree but we have to be a bit careful. People don't like "obviously harmless" UB to be exploited, e.g., passing null to a nonnull argument that is unused should not cause an unreachable

Right, I hadn't thought of that.

[baziotis]

Fixed test which actually exposed a FIXME. Again for the same reason, it doesn't seem to get a known value from AAValueSimplify.
Should I go ahead and change to unreachable the assumed instructions as well? Meaning in manifest(), after the updates have finished.

[jdoerfert]

In D71960#1877443, @baziotis wrote:

Fixed test which actually exposed a FIXME. Again for the same reason, it doesn't seem to get a known value from AAValueSimplify.
Should I go ahead and change to unreachable the assumed instructions as well? Meaning in manifest(), after the updates have finished.

Yes, I think something like that makes sense. Let's do that in a follow up though. This looks good now and I'll commit it once I get the chance.

[baziotis]

In D71960#1883463, @jdoerfert wrote:

In D71960#1877443, @baziotis wrote:

Fixed test which actually exposed a FIXME. Again for the same reason, it doesn't seem to get a known value from AAValueSimplify.
Should I go ahead and change to unreachable the assumed instructions as well? Meaning in manifest(), after the updates have finished.

Yes, I think something like that makes sense. Let's do that in a follow up though. This looks good now and I'll commit it once I get the chance.

Yes, ok, thanks!

[baziotis]

• Rebased and ready for commit.

The improved argument promotion made the test load_null_propagated2() in undefined_behavior.ll useless.
So, if we're all ok with that, I'm committing.

[baziotis]

• Rebase and ping :)

Note: I have commit access now, just making sure you agree with the (small) changes in the tests.

[baziotis]

Can I get a look on this?

[jdoerfert]

LGTM

[baziotis]

• Final update

p.s Ty Johannes

[baziotis]

Committed but of course I forgot again to add the differential revision. I don't why I continuously do that, sorry. :)
I'll add it to the commit in Phabricator and close this when the commit is ready.

[baziotis]

Closed by: https://reviews.llvm.org/rGf3dd3a66d3d5643a5e6f46bb18dee3be3a5898cd

llvm/lib/Transforms/IPO/Attributor.cpp

auto InspectMemAccessInstForUB = [&](Instruction &I) {
 
       // If we reach here, we know we have an instruction
       // that accesses memory through a pointer operand,
       // for which getPointerOperand() should give it to us.
       const Value *PtrOp = getPointerOperand(&I, /* AllowVolatile */ true);
       assert(PtrOp &&
              "Expected pointer operand of memory accessing instruction");
 
+      // Either we stopped and the appropriate action was taken,
+      // or we got back a simplified value to continue.
+      Optional<Value *> SimplifiedPtrOp = stopOnUndefOrAssumed(A, PtrOp, &I);
+      if (!SimplifiedPtrOp.hasValue())
+        return true;
+      const Value *PtrOpVal = SimplifiedPtrOp.getValue();
+
       // A memory access through a pointer is considered UB
       // only if the pointer has constant null value.
       // TODO: Expand it to not only check constant values.
-      if (!isa<ConstantPointerNull>(PtrOp)) {
+      if (!isa<ConstantPointerNull>(PtrOpVal)) {
         AssumedNoUBInsts.insert(&I);
         return true;
       }
-      const Type *PtrTy = PtrOp->getType();
+      const Type *PtrTy = PtrOpVal->getType();
 
       // Because we only consider instructions inside functions,
       // assume that a parent function exists.
       const Function *F = I.getFunction();
 
       // A memory access using constant null pointer is only considered UB
       // if null pointer is _not_ defined for the target platform.
       if (llvm::NullPointerIsDefined(F, PtrTy->getPointerAddressSpace()))

llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
+; RUN: opt -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 -S < %s | FileCheck %s
 ; PR2498
 
 ; This test tries to convince CHECK about promoting the load from %A + 2,
 ; because there is a load of %A in the entry block
 define internal i32 @callee(i1 %C, i32* %A) {
-; CHECK-LABEL: define {{[^@]+}}@callee()
+; CHECK-LABEL: define {{[^@]+}}@callee
+; CHECK-SAME: (i32* nocapture nofree nonnull readonly align 4 dereferenceable(4) [[A:%.*]])
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[A_0:%.*]] = load i32, i32* null, align 536870912
+; CHECK-NEXT:    [[A_0:%.*]] = load i32, i32* %A
 ; CHECK-NEXT:    br label [[F:%.*]]
 ; CHECK:       T:
 ; CHECK-NEXT:    unreachable
 ; CHECK:       F:
-; CHECK-NEXT:    [[A_2:%.*]] = getelementptr i32, i32* null, i32 2
-; CHECK-NEXT:    [[R:%.*]] = load i32, i32* [[A_2]], align 4
+; CHECK-NEXT:    [[A_2:%.*]] = getelementptr i32, i32* %A, i32 2
+; CHECK-NEXT:    [[R:%.*]] = load i32, i32* [[A_2]]
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
 entry:
   ; Unconditonally load the element at %A
   %A.0 = load i32, i32* %A
   br i1 %C, label %T, label %F
 
 T:
   ret i32 %A.0
 
 F:
   ; Load the element at offset two from %A. This should not be promoted!
   %A.2 = getelementptr i32, i32* %A, i32 2
   %R = load i32, i32* %A.2
   ret i32 %R
 }
 
-define i32 @foo() {
-; CHECK-LABEL: define {{[^@]+}}@foo()
-; CHECK-NEXT:    [[X:%.*]] = call i32 @callee()
+define i32 @foo(i32* %A) {
+; CHECK-LABEL: define {{[^@]+}}@foo(
+; CHECK-NEXT:    [[X:%.*]] = call i32 @callee(i32* nocapture nofree readonly align 4 %A)
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
-  %X = call i32 @callee(i1 false, i32* null)             ; <i32> [#uses=1]
+  %X = call i32 @callee(i1 false, i32* %A)             ; <i32> [#uses=1]
   ret i32 %X
 }
 

llvm/test/Transforms/Attributor/ArgumentPromotion/X86/attributes.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 ; Test that we only promote arguments when the caller/callee have compatible
 ; function attrubtes.
 
 target triple = "x86_64-unknown-linux-gnu"
 
 define internal fastcc void @no_promote_avx2(<4 x i64>* %arg, <4 x i64>* readonly %arg1) #0 {
 ; CHECK-LABEL: define {{[^@]+}}@no_promote_avx2
 ; CHECK-SAME: (<4 x i64>* noalias nocapture nofree nonnull writeonly align 32 dereferenceable(32) [[ARG:%.*]], <4 x i64>* noalias nocapture nofree nonnull readonly align 32 dereferenceable(32) [[ARG1:%.*]])

llvm/test/Transforms/Attributor/ArgumentPromotion/X86/min-legal-vector-width.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 ; Test that we only promote arguments when the caller/callee have compatible
 ; function attrubtes.
 
 target triple = "x86_64-unknown-linux-gnu"
 
 ; This should promote
 define internal fastcc void @callee_avx512_legal512_prefer512_call_avx512_legal512_prefer512(<8 x i64>* %arg, <8 x i64>* readonly %arg1) #0 {
 ; CHECK-LABEL: define {{[^@]+}}@callee_avx512_legal512_prefer512_call_avx512_legal512_prefer512

llvm/test/Transforms/Attributor/ArgumentPromotion/alignment.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 
 define void @f() {
 ; CHECK-LABEL: define {{[^@]+}}@f()
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 1
 ; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* [[A]], align 1
 ; CHECK-NEXT:    call void @g(i32 [[TMP0]])
 ; CHECK-NEXT:    ret void

llvm/test/Transforms/Attributor/ArgumentPromotion/control-flow.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 
 ; Don't promote around control flow.
 define internal i32 @callee(i1 %C, i32* %P) {
 ; CHECK-LABEL: define {{[^@]+}}@callee
 ; CHECK-SAME: (i1 [[C:%.*]], i32* nocapture nofree readonly [[P:%.*]])
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br i1 [[C]], label [[T:%.*]], label [[F:%.*]]
 ; CHECK:       T:

llvm/test/Transforms/Attributor/ArgumentPromotion/fp80.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 %union.u = type { x86_fp80 }
 %struct.s = type { double, i16, i8, [5 x i8] }
 
 @b = internal global %struct.s { double 3.14, i16 9439, i8 25, [5 x i8] undef }, align 16

llvm/test/Transforms/Attributor/ArgumentPromotion/profile.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
 
 ; Checks if !prof metadata is corret in deadargelim.
 
 define void @caller() #0 {
 ; CHECK-LABEL: define {{[^@]+}}@caller()
 ; CHECK-NEXT:    [[X:%.*]] = alloca i32
 ; CHECK-NEXT:    store i32 42, i32* [[X]], align 4

llvm/test/Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 
 ; PR17906
 ; When we promote two arguments in a single function with different types,
 ; before the fix, we used the same tag for the newly-created two loads.
 ; This testing case makes sure that we correctly transfer the tbaa tags from the
 ; original loads to the newly-created loads when promoting pointer arguments.
 
 @a = global i32* null, align 8

llvm/test/Transforms/Attributor/ArgumentPromotion/sret.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes='attributor' -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=4 < %s | FileCheck %s
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc"
 
 define internal void @add({i32, i32}* %this, i32* sret %r) {
 ; CHECK-LABEL: define {{[^@]+}}@add
 ; CHECK-SAME: ({ i32, i32 }* noalias nocapture nofree nonnull readonly align 8 dereferenceable(8) [[THIS:%.*]], i32* noalias nocapture nofree nonnull sret writeonly align 4 dereferenceable(4) [[R:%.*]])
 ; CHECK-NEXT:    [[AP:%.*]] = getelementptr { i32, i32 }, { i32, i32 }* [[THIS]], i32 0, i32 0

llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=5 < %s | FileCheck %s
+; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=6 < %s | FileCheck %s
 ; Don't constant-propagate byval pointers, since they are not pointers!
 ; PR5038
 %struct.MYstr = type { i8, i32 }
 @mystr = internal global %struct.MYstr zeroinitializer ; <%struct.MYstr*> [#uses=3]
 define internal void @vfu1(%struct.MYstr* byval align 4 %u) nounwind {
 entry:
   %0 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1 ; <i32*> [#uses=1]
   store i32 99, i32* %0, align 4

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
 ; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 < %s | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
-define void @fn2(i32* %P) {
+define void @fn2(i32* %P, i1 %C) {
 ; CHECK-LABEL: define {{[^@]+}}@fn2
-; CHECK-SAME: (i32* nocapture nofree writeonly [[P:%.*]])
+; CHECK-SAME: (i32* nocapture nofree [[P:%.*]], i1 %C)
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[IF_END:%.*]]
 ; CHECK:       for.cond1:
-; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    br i1 %C, label %if.end, label %exit
 ; CHECK:       if.end:
-; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* undef, align 4
+; CHECK-NEXT:    [[E_2:%.*]] = phi i32* [ %P, %entry ], [ null, %for.cond1 ]
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* [[E_2]], align 4
 ; CHECK-NEXT:    [[CALL:%.*]] = call i32 @fn1(i32 [[TMP0]])
 ; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]]
-; CHECK-NEXT:    br label [[FOR_COND1:%.*]]
+; CHECK-NEXT:    br label %for.cond1
 ;
 entry:
   br label %if.end
 
-for.cond1:                                        ; preds = %if.end, %for.end
-  br i1 undef, label %if.end, label %if.end
+for.cond1:                                        ; preds = %if.end
+  br i1 %C, label %if.end, label %exit
 
-if.end:                                           ; preds = %lbl, %for.cond1
-  %e.2 = phi i32* [ undef, %entry ], [ null, %for.cond1 ], [ null, %for.cond1 ]
+if.end:                                           ; preds = %entry, %for.cond1
+  %e.2 = phi i32* [ %P, %entry ], [ null, %for.cond1 ]
   %0 = load i32, i32* %e.2, align 4
   %call = call i32 @fn1(i32 %0)
   store i32 %call, i32* %P
   br label %for.cond1
+exit:
+  ret void
 }
 
 define internal i32 @fn1(i32 %p1) {
 ; CHECK-LABEL: define {{[^@]+}}@fn1
 ; CHECK-SAME: (i32 returned [[P1:%.*]])
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[P1]], 0
 ; CHECK-NEXT:    [[COND:%.*]] = select i1 [[TOBOOL]], i32 [[P1]], i32 [[P1]]
 ; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %tobool = icmp ne i32 %p1, 0
   %cond = select i1 %tobool, i32 %p1, i32 %p1
   ret i32 %cond
 }
 
-define void @fn_no_null_opt(i32* %P) #0 {
+define void @fn_no_null_opt(i32* %P, i1 %C) "null-pointer-is-valid"="true" {
 ; CHECK-LABEL: define {{[^@]+}}@fn_no_null_opt
-; CHECK-SAME: (i32* nocapture nofree writeonly [[P:%.*]])
+; CHECK-SAME: (i32* nocapture nofree writeonly [[P:%.*]], i1 %C)
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[IF_END:%.*]]
 ; CHECK:       for.cond1:
-; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    br i1 %C, label %if.end, label %exit
 ; CHECK:       if.end:
-; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* undef, align 4
+; CHECK-NEXT:    [[E_2:%.*]] = phi i32* [ undef, %entry ], [ null, %for.cond1 ]
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* null, align 4
 ; CHECK-NEXT:    [[CALL:%.*]] = call i32 @fn0(i32 [[TMP0]])
 ; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]]
-; CHECK-NEXT:    br label [[FOR_COND1:%.*]]
+; CHECK-NEXT:    br label %for.cond1
 ;
 entry:
   br label %if.end
 
-for.cond1:                                        ; preds = %if.end, %for.end
-  br i1 undef, label %if.end, label %if.end
+for.cond1:                                        ; preds = %if.end
+  br i1 %C, label %if.end, label %exit
 
-if.end:                                           ; preds = %lbl, %for.cond1
-  %e.2 = phi i32* [ undef, %entry ], [ null, %for.cond1 ], [ null, %for.cond1 ]
+if.end:                                           ; preds = %entry, %for.cond1
+  %e.2 = phi i32* [ undef, %entry ], [ null, %for.cond1 ]
   %0 = load i32, i32* %e.2, align 4
   %call = call i32 @fn0(i32 %0)
   store i32 %call, i32* %P
   br label %for.cond1
+exit:
+  ret void
 }
 
 define internal i32 @fn0(i32 %p1) {
 ; CHECK-LABEL: define {{[^@]+}}@fn0
 ; CHECK-SAME: (i32 returned [[P1:%.*]])
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[P1]], 0
 ; CHECK-NEXT:    [[COND:%.*]] = select i1 [[TOBOOL]], i32 [[P1]], i32 [[P1]]
 ; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %tobool = icmp ne i32 %p1, 0
   %cond = select i1 %tobool, i32 %p1, i32 %p1
   ret i32 %cond
 }
-
-attributes #0 = { "null-pointer-is-valid"="true" }

llvm/test/Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
-; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=2 < %s | FileCheck %s
+; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=3 < %s | FileCheck %s
 ;
 ;    void bar(int, float, double);
 ;
 ;    void foo(int N) {
 ;      float p = 3;
 ;      double q = 5;
 ;      N = 7;
 ;

llvm/test/Transforms/Attributor/internal-noalias.ll

-; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=6 < %s | FileCheck %s
+; RUN: opt -S -passes=attributor -aa-pipeline='basic-aa' -attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=7 < %s | FileCheck %s
 
 define dso_local i32 @visible(i32* noalias %A, i32* noalias %B) #0 {
 entry:
   %call1 = call i32 @noalias_args(i32* %A, i32* %B)
   %call2 = call i32 @noalias_args_argmem(i32* %A, i32* %B)
   %add = add nsw i32 %call1, %call2
   ret i32 %add
 }

llvm/test/Transforms/Attributor/returned.ll

-; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=5 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
+; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
 ; RUN: opt -attributor -attributor-manifest-internal -attributor-disable=false -attributor-annotate-decl-cs -functionattrs -S < %s | FileCheck %s --check-prefix=BOTH
 ;
 ; Copied from Transforms/FunctoinAttrs/read_write_returned_arguments_scc.ll
 ; 
 ; Test cases specifically designed for the "returned" argument attribute.
 ; We use FIXME's to indicate problems and missing attributes.
 ;
 

llvm/test/Transforms/Attributor/undefined_behavior.ll

;
   br i1 %cond, label %t, label %e
 t:
   %b = load i32, i32* null
   br label %e
 e:
   ret void
 }
 
+; Note that while the load is removed (because it's unused), the block
+; is not changed to unreachable
 define void @load_null_pointer_is_defined() "null-pointer-is-valid"="true" {
 ; ATTRIBUTOR-LABEL: @load_null_pointer_is_defined(
 ; ATTRIBUTOR-NEXT:    ret void
 ;
   %a = load i32, i32* null
   ret void
 }
 
 define internal i32* @ret_null() {
   ret i32* null
 }
 
-; FIXME: null is propagated but the instruction
-; is not changed to unreachable.
-define i32 @load_null_propagated() {
+define void @load_null_propagated() {
 ; ATTRIBUTOR-LABEL: @load_null_propagated(
-; ATTRIBUTOR-NEXT:    [[A:%.*]] = load i32, i32* null
-; ATTRIBUTOR-NEXT:    ret i32 [[A]]
+; ATTRIBUTOR-NEXT:    unreachable
 ;
   %ptr = call i32* @ret_null()
   %a = load i32, i32* %ptr
-  ret i32 %a
+  ret void
 }
 
 ; -- Store tests --
 
 define void @store_wholly_unreachable() {
 ; ATTRIBUTOR-LABEL: @store_wholly_unreachable(
 ; ATTRIBUTOR-NEXT:    unreachable
 ;
   store i32 5, i32* null
   ret void
 }
 
 define void @store_single_bb_unreachable(i1 %cond) {
 ; ATTRIBUTOR-LABEL: @store_single_bb_unreachable(
 ; ATTRIBUTOR-NEXT:    br i1 [[COND:%.*]], label [[T:%.*]], label [[E:%.*]]
 ; ATTRIBUTOR:       t:
 ; ATTRIBUTOR-NEXT:    unreachable
 ; ATTRIBUTOR:       e:
 ; ATTRIBUTOR-NEXT:    ret void
 ;
   br i1 %cond, label %t, label %e
 t:
   store i32 5, i32* null
   br label %e
 e:
   ret void
 }
 
 define void @store_null_pointer_is_defined() "null-pointer-is-valid"="true" {
 ; ATTRIBUTOR-LABEL: @store_null_pointer_is_defined(
 ; ATTRIBUTOR-NEXT:    store i32 5, i32* null
 ; ATTRIBUTOR-NEXT:    ret void
 ;
   store i32 5, i32* null
   ret void
 }
 
+define void @store_null_propagated() {
+; ATTRIBUTOR-LABEL: @store_null_propagated(
+; ATTRIBUTOR-NEXT:    unreachable
+;
+  %ptr = call i32* @ret_null()
+  store i32 5, i32* %ptr
+  ret void
+}
+
 ; -- AtomicRMW tests --
 
 define void @atomicrmw_wholly_unreachable() {
 ; ATTRIBUTOR-LABEL: @atomicrmw_wholly_unreachable(
 ; ATTRIBUTOR-NEXT:    unreachable
 ;
   %a = atomicrmw add i32* null, i32 1 acquire
   ret void

 ; ATTRIBUTOR-LABEL: @atomicrmw_null_pointer_is_defined(
 ; ATTRIBUTOR-NEXT:    [[A:%.*]] = atomicrmw add i32* null, i32 1 acquire
 ; ATTRIBUTOR-NEXT:    ret void
 ;
   %a = atomicrmw add i32* null, i32 1 acquire
   ret void
 }
 
+define void @atomicrmw_null_propagated() {
+; ATTRIBUTOR-LABEL: @atomicrmw_null_propagated(
+; ATTRIBUTOR-NEXT:    unreachable
+;
+  %ptr = call i32* @ret_null()
+  %a = atomicrmw add i32* %ptr, i32 1 acquire
+  ret void
+}
+
 ; -- AtomicCmpXchg tests --
 
 define void @atomiccmpxchg_wholly_unreachable() {
 ; ATTRIBUTOR-LABEL: @atomiccmpxchg_wholly_unreachable(
 ; ATTRIBUTOR-NEXT:    unreachable
 ;
   %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
   ret void

 ; ATTRIBUTOR-LABEL: @atomiccmpxchg_null_pointer_is_defined(
 ; ATTRIBUTOR-NEXT:    [[A:%.*]] = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
 ; ATTRIBUTOR-NEXT:    ret void
 ;
   %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
   ret void
 }
 
+define void @atomiccmpxchg_null_propagated() {
+; ATTRIBUTOR-LABEL: @atomiccmpxchg_null_propagated(
+; ATTRIBUTOR-NEXT:    unreachable
+;
+  %ptr = call i32* @ret_null()
+  %a = cmpxchg i32* %ptr, i32 2, i32 3 acq_rel monotonic
+  ret void
+}
+
+; -- Conditional branching tests --
+
 ; Note: The unreachable on %t and %e is _not_ from AAUndefinedBehavior
 
 define i32 @cond_br_on_undef() {
 ; ATTRIBUTOR-LABEL: @cond_br_on_undef(
 ; ATTRIBUTOR-NEXT:    unreachable
 ; ATTRIBUTOR:       t:
 ; ATTRIBUTOR-NEXT:    unreachable
 ; ATTRIBUTOR:       e: