This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Use the assumption that if the pointer was loaded/stored, then it is nonnull.
ClosedPublic

Authored by danlark on Dec 8 2019, 12:13 PM.

Details

Summary

If the pointer was loaded/stored before the null check, the check is redundant and can be removed. For now the optimizers do not remove the nullptr check, see https://gcc.godbolt.org/z/H2r5GG. The patch allows to use more nonnull constraints. Also, it found one more optimization in some PowerPC test. This is my first llvm review, I am free to any comments.

Also, I don't have commit rights.

Patch by Danila Kutenin.

Event Timeline

danlark created this revision.Dec 8 2019, 12:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2019, 12:13 PM
Herald added a subscriber: wuzish. · View Herald Transcript
lebedev.ri added inline comments.Dec 8 2019, 12:21 PM
llvm/lib/Analysis/ValueTracking.cpp
1928

This is certainly not sound for non-default address spaces;
does this already check for default address space somewhere here?

danlark added inline comments.Dec 8 2019, 12:30 PM
llvm/lib/Analysis/ValueTracking.cpp
1928

I don't see any checks of this. Should I add here?

danlark updated this revision to Diff 232742.Dec 8 2019, 12:52 PM

Checking the address space to be default.

danlark marked 3 inline comments as done.Dec 8 2019, 12:53 PM
danlark added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
1928

Done

There were behavior-changing alterations, but no test changes?

llvm/lib/Analysis/ValueTracking.cpp
1928

You want NullPointerIsDefined()

There were behavior-changing alterations, but no test changes?

If I understand correctly, yes.

There were behavior-changing alterations, but no test changes?

If I understand correctly, yes.

It was another way of saying: "there's no test coverage for that change, it'll break and no one will notice until it miscompiles, tests should be added"

There were behavior-changing alterations, but no test changes?

If I understand correctly, yes.

It was another way of saying: "there's no test coverage for that change, it'll break and no one will notice until it miscompiles, tests should be added"

test_null_after_{load,store} fail without that change

danlark updated this revision to Diff 232745.Dec 8 2019, 1:18 PM

Check NullPointerIsDefined function for address spaces

danlark marked an inline comment as done.Dec 8 2019, 1:22 PM
danlark updated this revision to Diff 232746.Dec 8 2019, 1:30 PM

Remove nonnull attribute from memcpy-addrspace test

There were behavior-changing alterations, but no test changes?

If I understand correctly, yes.

It was another way of saying: "there's no test coverage for that change, it'll break and no one will notice until it miscompiles, tests should be added"

test_null_after_{load,store} fail without that change

What i'm asking is - are there tests that ensure we don't erroneously deduce nonnull
from load/store when NullPointerIsDefined() returned true? (there should be such tests).
Also, DT->dominates() could use a few more tests.

danlark updated this revision to Diff 232747.Dec 8 2019, 1:59 PM

Add more tests on different address spaces and instruction dominating

There were behavior-changing alterations, but no test changes?

If I understand correctly, yes.

It was another way of saying: "there's no test coverage for that change, it'll break and no one will notice until it miscompiles, tests should be added"

test_null_after_{load,store} fail without that change

What i'm asking is - are there tests that ensure we don't erroneously deduce nonnull
from load/store when NullPointerIsDefined() returned true? (there should be such tests).
Also, DT->dominates() could use a few more tests.

Added!

danlark edited the summary of this revision. (Show Details)Dec 8 2019, 4:01 PM

This seems correct.

"Similar" functionality will be introduced into the Attributor with D68934 (I also copied the new tests). However, that should not stop this from going forward.

danlark edited the summary of this revision. (Show Details)Dec 9 2019, 12:33 AM
uenoku added a subscriber: uenoku.Dec 9 2019, 8:33 AM
danlark edited the summary of this revision. (Show Details)Dec 9 2019, 8:51 AM

Also, I don't have commit rights.

@lebedev.ri From my perspective this is fine. Feel free to accept and commit this.

Also, I don't have commit rights.

@lebedev.ri From my perspective this is fine. Feel free to accept and commit this.

I also currently don't have any further counter-examples here.

@danlark please specify who <e@ma.il> to be used for the change.

Also, I don't have commit rights.

@lebedev.ri From my perspective this is fine. Feel free to accept and commit this.

I also currently don't have any further counter-examples here.

@danlark please specify who <e@ma.il> to be used for the change.

I don't get what you mean, in the commit message it is said to state my name and that's all. https://llvm.org/docs/DeveloperPolicy.html#commit-messages

danlark edited the summary of this revision. (Show Details)Dec 9 2019, 12:29 PM
danlark edited the summary of this revision. (Show Details)Dec 9 2019, 12:38 PM

Also, I don't have commit rights.

@lebedev.ri From my perspective this is fine. Feel free to accept and commit this.

I also currently don't have any further counter-examples here.

@danlark please specify who <e@ma.il> to be used for the change.

I don't get what you mean, in the commit message it is said to state my name and that's all. https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Now that the repo is using git, it is possible to not just mention the original patch's author textually,
but actually specify the actual git commit author, #commit-messages isn't up-to-date about that.

Also, I don't have commit rights.

@lebedev.ri From my perspective this is fine. Feel free to accept and commit this.

I also currently don't have any further counter-examples here.

@danlark please specify who <e@ma.il> to be used for the change.

I don't get what you mean, in the commit message it is said to state my name and that's all. https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Now that the repo is using git, it is possible to not just mention the original patch's author textually,
but actually specify the actual git commit author, #commit-messages isn't up-to-date about that.

My github account is https://github.com/danlark1, email is kutdanila at yandex.ru

danlark edited the summary of this revision. (Show Details)Dec 9 2019, 12:49 PM
danlark updated this revision to Diff 232913.Dec 9 2019, 12:49 PM
This comment was removed by danlark.
danlark updated this revision to Diff 233265.EditedDec 11 2019, 12:27 AM

As D71181 was submitted, I need to change some things as my code relied on function accepting pointer.

Harbormaster completed remote builds in B42278: Diff 233266.
nikic added inline comments.Dec 11 2019, 12:35 AM
llvm/lib/Analysis/ValueTracking.cpp
1914

I don't think this makes sense. Getting the address space is not expensive. You can just refetch it below.

Alternatively, if you really do want to extract it, then the whole NullPointerIsDefined check should be extracted.

danlark updated this revision to Diff 233269.Dec 11 2019, 12:37 AM
Extract address space inside the condition
danlark marked an inline comment as done.Dec 11 2019, 12:38 AM
This comment was removed by danlark.
danlark marked an inline comment as done.Dec 11 2019, 12:38 AM
danlark added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
1914

Makes sense, done

As two people agreed on the patch in the comments, waiting for LGTM and submit from somebody else (email and github account are in the comments above)

This revision is now accepted and ready to land.Dec 11 2019, 11:31 AM
This revision was automatically updated to reflect the committed changes.