Page MenuHomePhabricator

Express and handle specific info about the processing of invariant intrinsics
Needs ReviewPublic

Authored by lvoufo on Oct 22 2015, 8:57 PM.

Details

Summary

Add an 'InvariantInfo' property to the 'Module' data structure, and use the property to extend basic AA's assessment that a given memory location is to be treated as constant memory.

The 'InvariantInfo' field in 'Module' will be used to collect information meaningful for load elimination while processing invariant intrinsics calls.

This patch is a break-down of (corrected) D13606.

Diff Detail

Event Timeline

lvoufo updated this revision to Diff 38207.Oct 22 2015, 8:57 PM
lvoufo retitled this revision from to Extend LLVM with info about the processing of invariant intrinsics.
lvoufo updated this object.
lvoufo added reviewers: nlewycky, dberlin, reames.
lvoufo added a subscriber: llvm-commits.
lvoufo retitled this revision from Extend LLVM with info about the processing of invariant intrinsics to Express and handle specific info about the processing of invariant intrinsics.

I split this up into D14008 and D14009. I hope that this does not cause too much of an inconvenience. Please let me know...
--L

sanjoy added a subscriber: sanjoy.Oct 22 2015, 9:53 PM
sanjoy added inline comments.
include/llvm/IR/InvariantInfo.h
27

Please add some high level comments describing how this is supposed to be used.

28

Please add some comments on how you deal with passes RAUW'ing or erasing values.

55

Please document these functions.

lvoufo added inline comments.Oct 22 2015, 9:57 PM
include/llvm/IR/InvariantInfo.h
55

Do you mean in addition to the documentation of its implementation in lib/IR/InvariantInfo.cpp ?

sanjoy added inline comments.Oct 22 2015, 9:58 PM
include/llvm/IR/InvariantInfo.h
55

Never mind, I just saw the docs in the implementation file. :)

The doxygen comments are typically put in the header.

lvoufo added inline comments.Oct 22 2015, 10:04 PM
include/llvm/IR/InvariantInfo.h
27

Ok.

28

Ok.

55

No problem. I just figured it would be more legible to have the detailed documentation right next to the implementation. I can add some high-level comments on the prototypes too if it help.

lvoufo updated this revision to Diff 38212.Oct 22 2015, 10:31 PM
lvoufo marked 8 inline comments as done.
nlewycky added inline comments.Nov 8 2015, 4:23 PM
include/llvm/IR/InvariantInfo.h
22–24

Alphabetize.

26–28

One side says "PreservedInvariantInfo" the other side says "InvariantInfo". Just remove the class name from the comment, that style is pre-style guide.

Also, "a data structure to mark and access processed invariant intrinsic calls"? I don't think that tells me what it does or what it's for. Is it a cache of invariant intrinsics that pertain to a given Value?

30

"writeonce" is not defined anywhere?

31

Typo in "intrisic_start"

47

Remove struct name from comment.

59

Hold on, you have one struct PreservedInvariantInfo and one class PreserveInvariantInfo? Please rename one of them.

include/llvm/IR/Module.h
177

A Module has a 1:1 correlation with the .ll and the .bc. This shouldn't go there, probably it should be an analysis hanging off the side like AssumptionCache is.

lib/Analysis/BasicAliasAnalysis.cpp
499

This seems like a really bad interface. The Pass which sets up the InvInfo has to decide whether to add or not add an invariant intrinsic based on which instruction the caller of pointsToConstantMemory is going to be reasoning about. pointsToConstantMemory could be called by a utility used by the pass, and referring to a point in the program before (or after) the call to invariant.start/end.

lib/IR/InvariantInfo.cpp
2

Remove the "-*- C++ -*-" part, that's only for .h files.

reames resigned from this revision.Apr 18 2016, 4:56 PM
reames removed a reviewer: reames.