[PATCH] D15124: Use @llvm.invariant.start/end intrinsics to extend basic AA with invariant range analysis for GVN-based load elimination purposes [Local objects only]
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 12 20:15:37 PST 2016
chandlerc added a comment.
First pass of comments. Also, see my reply to Hal on the earlier thread as pertains to post-dominance information.
While many of these issues are small, there are a couple of pretty significant design comments below. Happy to discuss those to try to make sure you have the right design before you work on another iteration.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:71
@@ +70,3 @@
+ /// \brief Sets the dominator and post-dominator trees.
+ void init(DominatorTree *DomTree, PostDominatorTreeT *PostDomTree) {
+ // If the dominator and postdominator trees have changed, then
----------------
Using an init function is not terribly common in LLVM. Why not use a constructor?
================
Comment at: include/llvm/Analysis/InvariantInfo.h:81
@@ +80,3 @@
+ /// \brief Enables invariant range analysis.
+ void EnableInvariantRangeAnalysis(bool DoEnable = true) {
+ RangeAnalysisIsEnabled = DoEnable;
----------------
The style of this patch seems mixed. Sometimes methods start with a lower case, some times an upper case. Please try to consistently follow the LLVM coding standards.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:85-96
@@ +84,14 @@
+
+ /// \brief Retrieves the part of the invariants mapping that is associated
+ /// with the given function.
+ InvListMapT &getInvariantsMap(const Function &F);
+
+ /// \brief Access the invariants mapping to extract invariant_start/end
+ /// instructions that are associated with the given address.
+ InvListT &getInvariants(const Function &F, const Value *Addr);
+
+ /// \brief Adds an entry to the invariants mapping.
+ void addInvariant(const Function &F, const Value *Addr,
+ const IntrinsicInst *IStart);
+
+ /// \brief Populates the invariants mapping for the given function.
----------------
Why are these public methods? they only appear to be called from within the implementation or from diagnostic utilities.
================
Comment at: include/llvm/Analysis/InvariantInfo.h:195-218
@@ +194,26 @@
+
+/// \brief The -invariant-range-analysis pass.
+class InvariantRangeAnalysisPass : public ImmutablePass {
+ InvariantInfo InvInfo;
+
+ public:
+ static char ID; // Pass identification
+ explicit InvariantRangeAnalysisPass();
+ ~InvariantRangeAnalysisPass() override;
+
+ InvariantInfo &getInvariantInfo() { return InvInfo; }
+ const InvariantInfo &getInvariantInfo() const { return InvInfo; }
+
+ void InitDependencies(FunctionPass &P);
+
+ void releaseMemory() override { InvInfo.clear(); }
+ void verifyAnalysis() const override;
+ void getAnalysisUsage(AnalysisUsage &AU) const override;
+ void print(raw_ostream &OS, const Module *) const override;
+ void dump() const;
+ bool doFinalization(Module &) override {
+ verifyAnalysis();
+ return false;
+ }
+};
+
----------------
Why use a pass at all? You don't seem to be getting much value from the pass infrastructure currently. It looks like currently, you could just have a function like "populateInfo" which returns an InvariantRangeInfo object after analyzing that function, and the InvariantRangeInfo object could have a method called "pointsToReadOnlyMemory"?
Do you have specific future changes that would require the pass infrastructure to work? It's entirely possible I'm missing something, but if so, it would be very helpful to explain this kind of detail in the comments.
================
Comment at: include/llvm/Analysis/PostDominators.h:131-148
@@ -114,1 +130,20 @@
+/// \brief Analysis pass which computes a \c PostDominatorTree.
+class PostDominatorTreeAnalysis {
+ public:
+ /// \brief Provide the result typedef for this analysis pass.
+ typedef PostDominatorTreeT Result;
+
+ /// \brief Opaque, unique identifier for this analysis pass.
+ static void *ID() { return (void *)&PassID; }
+
+ /// \brief Run the analysis pass over a function and produce a dominator tree.
+ PostDominatorTreeT run(Function &F);
+
+ /// \brief Provide access to a name for this pass for debugging purposes.
+ static StringRef name() { return "PostDominatorTreeAnalysis"; }
+
+ private:
+ static char PassID;
+};
+
----------------
Please split the port of postdom to the new pass manager into a separate patch. Even if it is truly necessary, the port shouldn't be part of the patch that introduces the invariant stuff.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:678
@@ +677,3 @@
+static bool isInvariantIntrinsic(ImmutableCallSite CS) {
+ return IsInvariantIntrinsic(dyn_cast<IntrinsicInst>(CS.getInstruction()));
+}
----------------
It isn't clear what value this helper function is providing.
Is it valid to pass a null pointer here? I find that surprising in and of itself.
Either way, I think it would be more clear to either teach the routine that is already provided by InvariantInfo.h to accept ImmutableCallSite objects, or to inline the extraction of the instruction into the places this is called below.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:742-743
@@ -736,1 +741,4 @@
+ // Invariant intrinsics follow the same pattern as assume intrinsic.
+ if (isInvariantIntrinsic(CS)) return MRI_NoModRef;
+
----------------
This formatting seems inconsistent. Run clang-format?
================
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;
+
----------------
You haven't addressed Nick's comment that teaching BasicAA to ignore the invariant intrinsic calls themselves is a separable patch that should be split out and submitted independently. Please do so.
================
Comment at: lib/Analysis/InvariantInfo.cpp:36-38
@@ +35,5 @@
+ if (!InvariantsComputed.count(&F)) populateInfo(F);
+ return AllInvariantsMaps[&F];
+}
+
+InvariantInfo::InvListT &InvariantInfo::getInvariants(const Function &F,
----------------
I don't really understand your concern here.
The comments are using imprecise language in that they're not using the standard's precise terminology, but the intent seems clear: it returns whatever "T()" produces. That *always* produces a null pointer when T is a pointer type.
All of LLVM's code (and much of Clang's) relies on the lookup method being reasonable to call when the result is a pointer. Please follow this convention as suggested by Nick originally.
================
Comment at: lib/Analysis/InvariantInfo.cpp:89-95
@@ +88,9 @@
+
+/// \brief An instruction cannot modify memory if the instruction is in an
+/// invariant range over the memory's pointer value.
+/// This implementation is modeled after \c pointsToConstantMemory(), with
+/// OrLocal == false, and uses \c queriedFromInvariantRange() to check if
+/// the given instruction modify the given memory.
+/// In practice, this function should be called immediately after
+/// \c pointsToConstantMemory() (with OrLocal == false) returns false.
+bool InvariantInfo::pointsToReadOnlyMemory(const Instruction *I,
----------------
You have two different API comments, one in the header, and one in the source. Please only have one comment. For public interfaces, it should be in the header. For private functions, you can put it near the implementation if you want.
================
Comment at: lib/Analysis/PostDominators.cpp:52-67
@@ +51,18 @@
+ // post-dominates the instruction.
+ // TODO: Special case for invoke instructions:
+ // Allow invoke instructions to be post-dominated by instructions in
+ // their normal destination path to allow further optimizations from
+ // invariant range analysis such as the elimination of the second load
+ // instruction in
+ //
+ // <code>
+ // try { ...
+ // @llvm.invariant.start(..., bitcast i32* %i to i8*)
+ // load %i
+ // invoke(...)
+ // load %i
+ // ...
+ // @llvm.invariant.end(..., bitcast i32* %i to i8*)
+ // } catch (...) { ... }
+ // </code>
+ //
----------------
This doesn't seem like it belongs in the postdom pass. Instead if anything it should live somewhere with the invariant handling code.
================
Comment at: lib/Transforms/Scalar/GVN.cpp:2441-2447
@@ +2440,9 @@
+
+ // Enable invariant range analysis here to keep its effect local to GVN.
+ auto &InvRA = getAnalysis<InvariantRangeAnalysisPass>();
+ InvInfo = &InvRA.getInvariantInfo();
+ InvInfo->EnableInvariantRangeAnalysis();
+
+ // Once enabled, initialize the analysis' dependencies.
+ InvRA.InitDependencies(*this);
+ }
----------------
This does not seem like the right design and seems over-engineered.
Here is an alternative that seems much simpler: pass your InvariantRangeAnalysis or InvariantRangeInfo in as an optional parameter to MemoryDependenceAnalysis::getSimplePointerDependencyFrom and the few APIs that call it. Then GVN can opt into building the invariant information structures and passing them into the query and other passes can skip that. Then, within memdep, where we query AA, also query the invariant analysis.
Perhaps there are other designs as well that would be simpler and more direct? I'm curious what others think.
http://reviews.llvm.org/D15124
More information about the llvm-commits
mailing list