[PATCH] D15124: Use @llvm.invariant.start/end intrinsics to extend the meaning of basic AA's pointsToConstantMemory(), for GVN-based load elimination purposes [Local objects only]
Nick Lewycky via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 8 20:55:54 PST 2015
nlewycky added inline comments.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:22-24
@@ +21,5 @@
+namespace llvm {
+class Value;
+class DataLayout;
+class IntrinsicInst;
+
----------------
Please alphabetize.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:29
@@ +28,3 @@
+
+ /// \brief A mapping of each 'writeonce' allocated memory (via a
+ /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
----------------
I don't understand the connection between "writeonce" (which doesn't seem to be defined anywhere? link?) and invariant.start/end. I have have as many invariant intrinsics over the same memory as I like, there's nothing 'once' about it. There's also no guarantee that the memory ever gets written to at any point in particular.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:30
@@ +29,3 @@
+ /// \brief A mapping of each 'writeonce' allocated memory (via a
+ /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
+ /// call. This is to be used by passes that exploit the write-once-ness of
----------------
Typo, "intrisic_start" should be "intrinsic_start".
================
Comment at: include/llvm/Analysis/InvariantInfo.h:40-41
@@ +39,4 @@
+ /// \brief Access the 'InvariantMarkers' mapping to extract invariant_start
+ /// instruction that is associated with the given Address.
+ IntrinsicInst *GetStartInstruction(const Value *Addr) const;
+
----------------
In the comment you say Address but the argument is named Addr. Either you meant to say "address" or "Addr" in the comment, or change "Addr" to "Address" in the function declaration.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:44
@@ +43,3 @@
+ /// \brief Remove an entry from the 'InvariantMarkers' mapping, if the
+ /// given addr meets certain preconditions.
+ /// Returns true if the entry is actually removed, and false if the
----------------
I looked up the "certain preconditions" and they appear to be "if Addr is a global or an alloca". Why those preconditions? Why not llvm::isIdentifiedObject, for example? Why aren't these preconditions mentioned on SetStartInstruction?
Ttrue if removed, false if preconditions are not met ... what if the preconditions are met and it's not removed (because it's not a member)?
================
Comment at: include/llvm/Analysis/InvariantInfo.h:57-58
@@ +56,4 @@
+
+ /// Information processed from invariant intrinsics,
+ /// for a given query instruction.
+ struct PreservedInfo {
----------------
Odd line wrapping?
================
Comment at: include/llvm/Analysis/InvariantInfo.h:81
@@ +80,3 @@
+
+/// \brief Handle invariant_start/end intrinsics when scanning intructions
+/// backward to either find available loads or look for pointer dependencies
----------------
Typo, "intructions" -> "instructions".
================
Comment at: include/llvm/Analysis/InvariantInfo.h:81-83
@@ +80,5 @@
+
+/// \brief Handle invariant_start/end intrinsics when scanning intructions
+/// backward to either find available loads or look for pointer dependencies
+/// from a given query instruction, based on preserved invariant info.
+bool BackwardScanInvariantIntrinsic(const IntrinsicInst *II,
----------------
nlewycky wrote:
> Typo, "intructions" -> "instructions".
The comment fails to explain what it does. Looking at the code, it appears to update InvInfo by unsetting the start instruction when II is a start. It also has an assert to check that things are sane when II is an end, but since assertions can be disabled, that's not really part of its functionality. It also returns true iff II is either start or end intrinsic.
It wasn't clear the first time through how closely tied this is to the operation of PreserveInvariantInfoRAII.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:491
@@ -490,1 +490,3 @@
+ // A pointer value associated to an invariant_start intrinsic call is
+ // considered to point to memory that is local to the function.
----------------
nlewycky wrote:
> I don't understand this comment. Globals, for instance, aren't local to the function.
"associated to" --> "associated with".
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:491-492
@@ -490,1 +490,4 @@
+ // A pointer value associated to an invariant_start intrinsic call is
+ // considered to point to memory that is local to the function.
+ if (InvInfo && InvInfo->GetStartInstruction(V)) {
----------------
I don't understand this comment. Globals, for instance, aren't local to the function.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:495
@@ +494,3 @@
+ const GlobalVariable *GV = dyn_cast<GlobalVariable>(V);
+ assert((isa<AllocaInst>(V) || (GV && !GV->isConstant())) &&
+ "Invariant info are associated only with alloca instructions or"
----------------
You assert on invariant.start with a constant global variable, but that isn't a verifier failure. You would assert if queried on this well-defined code, right?
declare {}* @llvm.invariant.start(i64 %size, i8* nocapture %ptr)
@gv = internal constant i8 0
define void @test() {
call {}* @llvm.invariant.start(i64 1, i8* @gv)
ret void
}
I agree the construct is not useful, either we should make it fail the verifier (this would mean auditing all parts of llvm which mark globals constant to ensure they eliminate all possible invariant intrinsic calls that use the global), or we should delete them in some optimization pass (instcombine).
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:496-497
@@ +495,4 @@
+ assert((isa<AllocaInst>(V) || (GV && !GV->isConstant())) &&
+ "Invariant info are associated only with alloca instructions or"
+ "non-constant global variables");
+ continue;
----------------
Please add a space after "or". As it is, the text from the two lines get concatenated producing "...instructions ornon-constant global...".
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:753-754
@@ -736,1 +752,4 @@
+ // Invariant intrinsics follow the same pattern as assume intrinsic.
+ if (isInvariantIntrinsic(CS)) return MRI_NoModRef;
+
----------------
This ...
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:768-770
@@ -748,1 +767,5 @@
+ // Invariant intrinsics follow the same pattern as assume intrinsic.
+ if (isInvariantIntrinsic(CS1) || isInvariantIntrinsic(CS2))
+ return MRI_NoModRef;
+
----------------
... and this look like solid changes on their own. Can you factor them out into their own review and write testcases for them?
================
Comment at: lib/Analysis/InvariantInfo.cpp:29
@@ +28,3 @@
+
+ // constant globals are also not marked.
+ if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Addr)) {
----------------
Capitalize.
================
Comment at: lib/Analysis/InvariantInfo.cpp:35-37
@@ +34,5 @@
+ // Retrieve the value from the markers map.
+ auto EntryIter = InvariantMarkers.find(const_cast<Value *>(Addr));
+ if (EntryIter != InvariantMarkers.end()) return EntryIter->second;
+ return nullptr;
+}
----------------
Does this code simplify to:
return InvariantMarkers.lookup(const_cast<Value *>(Addr));
?
================
Comment at: lib/Analysis/InvariantInfo.cpp:54-56
@@ +53,5 @@
+void InvariantInfo::SetStartInstruction(Value *Addr, IntrinsicInst *IStart) {
+ // Un-map the current entry, if valid, to ensure that the new value is
+ // inserted into the markers map.
+ if (!UnsetStartInstruction(Addr)) return;
+
----------------
Er, the code doesn't match the comment here, right? As written, this means that if it is set, then trying to set again will unset it. That doesn't ensure the new value is inserted.
================
Comment at: lib/Analysis/InvariantInfo.cpp:59
@@ +58,3 @@
+ assert(Addr && (isa<GlobalVariable>(Addr) || isa<AllocaInst>(Addr)) &&
+ "Value must be a nonnull global variable or alloca instruction.");
+ assert(IStart && IStart->getIntrinsicID() == Intrinsic::invariant_start &&
----------------
Extra space between words in in "alloca instruction".
================
Comment at: lib/Analysis/InvariantInfo.cpp:68-69
@@ +67,4 @@
+ // Actually insert the value into the map. This operation must be successful.
+ bool Inserted = InvariantMarkers.insert(std::make_pair(Addr, IStart)).second;
+ assert(Inserted && "Setting new marker failed.");
+}
----------------
You need to add a
(void)Inserted;
for people building with assertions off and -Wunused-variable on.
================
Comment at: lib/Analysis/InvariantInfo.cpp:118
@@ +117,3 @@
+ if (II->getIntrinsicID() == Intrinsic::invariant_start) {
+ llvm::Value *Addr = II->getArgOperand(1)->stripPointerCasts();
+ InvInfo.SetStartInstruction(Addr, II);
----------------
No need for llvm:: here, we're inside a .cpp with "using namespace llvm;".
================
Comment at: lib/Analysis/InvariantInfo.cpp:122
@@ +121,3 @@
+ } else if (II->getIntrinsicID() == Intrinsic::invariant_end) {
+ llvm::Value *Addr = II->getArgOperand(2)->stripPointerCasts();
+ if (InvInfo.GetStartInstruction(Addr))
----------------
Extra llvm::
================
Comment at: lib/Analysis/InvariantInfo.cpp:145
@@ +144,3 @@
+ } else if (II->getIntrinsicID() == Intrinsic::invariant_end) {
+ llvm::Value *IStart = II->getArgOperand(0)->stripPointerCasts();
+ assert(Preserved.Info.II != dyn_cast<IntrinsicInst>(IStart) &&
----------------
Extra llvm::
================
Comment at: lib/Analysis/InvariantInfo.cpp:149
@@ +148,3 @@
+ "preserved invariant_start, if preserved.");
+ return true;
+ }
----------------
Need "(void)IStart;" here.
================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:517-519
@@ -510,1 +516,5 @@
+ // Same for invariant intrinsics, which may also unset preserved
+ // invariant info.
+ if (BackwardScanInvariantIntrinsic(II, Preserved, InvInfo)) continue;
+ }
----------------
Walking ScanIt across an invariant intrinsic that doesn't appertain to QueryInst will not cause InvInfo to be updated. At that point, BasicAAResult::pointsToConstantMemory will produce wrong answers for those pointers.
================
Comment at: test/Transforms/LoadElim/invariant.ll:1
@@ +1,2 @@
+; RUN: opt < %s -gvn -S | FileCheck %s
+
----------------
Why are you creating a new test/Transforms subdirectory? The directories there are named for the passes, this should go into test/Transforms/GVN.
Also, this should be -basicaa -gvn to test your changes to -basicaa, right? Or do you want a separate testcase in test/Analysis/BasicAA for that?
http://reviews.llvm.org/D15124
More information about the llvm-commits
mailing list