[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]

Wed Dec 9 09:10:32 PST 2015

lvoufo marked 22 inline comments as done.

================
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
----------------
The design doc stated that this was all part of simulating 'writeonce' behavior in LLVM (+Clang)... 
Ideally, invariant.start instructions are generated right after first write (e.g. construction) and LLVM would reject any write after an invariant_start and before a corresponding invariant_end, but we are not reinforcing this in LLVM. Documentations like this are meant to specify (and clarify) the intended use of invariant.start/end (for simulating wirteonce semantics). 
If it makes things even clearer, I could update the LangRef to this effect later (?).

================
Comment at: include/llvm/Analysis/InvariantInfo.h:41-42
@@ +40,4 @@
+  /// instruction that is associated with the given Address.
+  IntrinsicInst *GetStartInstruction(const Value *Addr) const;
+
+  /// \brief Add an entry to the 'InvariantMarkers' mapping.
----------------
meant "address", not "Address".

================
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
----------------
nlewycky wrote:
> 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)?
Yeah. I think this is a case of poorly-named functions after code restructuring... I'll reorganize everything accordingly.

================
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:
> 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.
Right. This could be clearer.
I mentioned in earlier comments that I may be getting rid of this function altogether (in the next revision of this patch), if I can't rename it appropriately, as part of separating the extended logic from the order in which GVN processes instructions.

================
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)) {
----------------
nlewycky wrote:
> nlewycky wrote:
> > I don't understand this comment. Globals, for instance, aren't local to the function.
> "associated to" --> "associated with".
I think the operative word is "considered". 
This is merely an attempt to follow the current documentation structure of the function pointsToConstantMemory(). Its documentation states:

```
/// Returns whether the given pointer value points to memory that is local to                                                       
/// the function, with global constants being considered local to all                                                               
/// functions.                                                                                                                      
bool BasicAAResult::pointsToConstantMemory(const MemoryLocation &Loc,
                                           bool OrLocal) 
```

================
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"
----------------
nlewycky wrote:
> 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).
I have removed the assertion.
We just want to localized this extended logic to non-constant globals, and this should be checked in InvariantInfo::SetStartInstruction().

================
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;
+}
----------------
nlewycky wrote:
> Does this code simplify to:
>   return InvariantMarkers.lookup(const_cast<Value *>(Addr));
> ?
In theory, yes. But I like to edge on the side of safety with find() over lookup() when it comes to the default initialization of pointers (or other non-user-defined objects). 

The documentation of lookup says:
"lookup - Return the entry for the specified key, or a default constructed value if no such entry exists".
This explicitly returns nullptr, in place of a "default constructed value"---the specification of which could in practice change at any time, "if no such entry exists". 


================
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;
+
----------------
nlewycky wrote:
> 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.
Removed.

================
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);
----------------
nlewycky wrote:
> No need for llvm:: here, we're inside a .cpp with "using namespace llvm;".
Oops. Residuals from multiple restructurings...

================
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;
+    }
----------------
nlewycky wrote:
> 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.
How so? 
I think it will do exactly what we want. By manipulating invariant info here based on QueryInst, we are selectively choosing when to apply the extension in BasicAA.

* A load from %i within an invariant range for %i temporarily unsets the marking for %i.
* A load from %j within an invariant range for %i does nothing to the marking for %i.
* A load from %j within an invariant range for %i  **should** preserve the value of BasicAAResult::pointsToConstantMemory() for %j, whether it is nested within an invariant range for %j or not.
* Meanwhile, A load from %i within an invariant range for %i always leads to BasicAAResult::pointsToConstantMemory for %i == true.

What is missing?



http://reviews.llvm.org/D15124



