This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] UB Attribute now handles all instructions that access memory through a pointer
ClosedPublic

Authored by baziotis on Dec 20 2019, 2:53 PM.

Details

Summary

Follow-up on: https://reviews.llvm.org/D71435
We basically use checkForAllInstructions to loop through all the instructions in a function that access memory through a pointer: load, store, atomicrmw, atomiccmpxchg
Note that we can now use the getPointerOperand() that gets us the pointer operand for an instruction that belongs to the aforementioned set.

Question: This function returns nullptr if the instruction is volatile. Why?
Guess: Because if it is volatile, we don't want to do any transformation to it.

Another subtle point is that I had to add AtomicRMW, AtomicCmpXchg to initializeInformationCache(). Following checkAllInstructions() path, that
seemed the most reasonable place to add it and correct the fact that these instructions were ignored (they were not in OpcodeInstMap etc.). Is that ok?

Diff Detail

Event Timeline

baziotis created this revision.Dec 20 2019, 2:53 PM
Herald added a project: Restricted Project. · View Herald Transcript

Quick initial comment

llvm/lib/Transforms/IPO/Attributor.cpp
2033

I think you should be able to use CheckForAllReadWriteInstructions here. Then you don't need to add 2 additional cases in initializeInformationCache.

This looks generally fine to me. Let me know what you think about the two comments below.

llvm/lib/Transforms/IPO/Attributor.cpp
2033

We can do that, but you'll see other instructions here too. I'm fine with doing it step by step as well.

llvm/test/Transforms/Attributor/undefined_behavior.ll
139

Maybe it makes sense to run the update_test_checks script on this file.

baziotis marked 2 inline comments as done.Dec 20 2019, 3:46 PM
baziotis added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
2033

I think you should be able to use CheckForAllReadWriteInstructions here. Then you don't need to add 2 additional cases in initializeInformationCache.

Thanks Stefan! I hadn't noticed.

We can do that, but you'll see other instructions here too. I'm fine with doing it step by step as well.

Alright, I'll leave it as it is and re-introduce a TODO to not only check these instructions (i.e. make clear that other instructions will be added).

llvm/test/Transforms/Attributor/undefined_behavior.ll
139

Thanks, I didn't know about it.

baziotis updated this revision to Diff 234987.Dec 20 2019, 3:55 PM
  • llvm/utils/update_test_checks.py ran on undefined_behavior.ll test file.
  • TODO to check more instructions in the future.
jdoerfert accepted this revision.Dec 20 2019, 6:25 PM

LGTM. I'll commit them soon. You can continue on top if you find the time and motivation.

This revision is now accepted and ready to land.Dec 20 2019, 6:25 PM
sstefan1 accepted this revision.Dec 21 2019, 4:39 AM
sstefan1 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
2011

nit: I think you should update these comments (line 2021) as well.

baziotis marked an inline comment as done.Dec 21 2019, 6:26 AM

LGTM. I'll commit them soon. You can continue on top if you find the time and motivation.

Great, I'll continue with branches. Could you please answer this question:

Question: getPointerOperand() (meaning, Attributor's getPointerOperand()) returns nullptr if the instruction is volatile. Why?
Guess: Because if it is volatile, we don't want to do any transformation to it.

llvm/lib/Transforms/IPO/Attributor.cpp
2011

Yep, I missed that. I'm uploading a new diff, thanks again.

baziotis updated this revision to Diff 235017.Dec 21 2019, 6:27 AM

Comments update.

This revision was automatically updated to reflect the committed changes.