[PATCH] Review for hoisting and sinking of equivalent memory instruction (Instruction Merge Pass)
Gerolf Hoflehner
ghoflehner at apple.com
Mon Jun 16 01:24:37 PDT 2014
================
Comment at: lib/Transforms/Scalar/IM.cpp:13
@@ +12,3 @@
+//
+//
+//===----------------------------------------------------------------------===//
----------------
Philip Reames wrote:
> Documentation needs expanded. Examples. Reasoning. Profitability.
Ok on Doc. The test case has an example but I add that to the Doc.
================
Comment at: lib/Transforms/Scalar/IM.cpp:44
@@ +43,3 @@
+namespace {
+class IM : public FunctionPass {
+ AliasAnalysis *AA;
----------------
Philip Reames wrote:
> As said in email, this is not an acceptable name.
Will change to InstructionMerger
================
Comment at: lib/Transforms/Scalar/IM.cpp:56
@@ +55,3 @@
+
+ void setAliasAnalysis(AliasAnalysis *A) { AA = A; }
+ AliasAnalysis *getAliasAnalysis() const { return AA; }
----------------
Philip Reames wrote:
> These setters and getters add no value. Please remove them.
Agree. Usage and wrappers for MD and AA should be consistent.
================
Comment at: lib/Transforms/Scalar/IM.cpp:107
@@ +106,3 @@
+ MD->invalidateCachedPointerInfo(LI->getPointerOperand());
+ if (MD && Inst->getType()->getScalarType()->isPointerTy()) {
+ MD->invalidateCachedPointerInfo(Inst);
----------------
Philip Reames wrote:
> Duplicate MD check.
Nice. Removed.
================
Comment at: lib/Transforms/Scalar/IM.cpp:115
@@ +114,3 @@
+/// Return tail block of a diamond.
+BasicBlock *IM::getDiamondTail(BasicBlock *BB) {
+ assert(isDiamondHead(BB) && "Basic block is not head of a diamond");
----------------
Philip Reames wrote:
> You need to define "diamond" somewhere. The header comment is a good place.
>
> Why do you only handle diamonds here? It seems like most of this could work for any split or merge point.
The header comment together with the example will enlighten diamond.
Simplicity. Less complexity makes it easier to review and commit the initial framework. Larger scope with code motion, speculation and similar optimizations has the tendency of exposing tuning issues across phases.
================
Comment at: lib/Transforms/Scalar/IM.cpp:117
@@ +116,3 @@
+ assert(isDiamondHead(BB) && "Basic block is not head of a diamond");
+ assert(isa<BranchInst>(BB->getTerminator()) &&
+ "Basic block is not a diamond head\n");
----------------
Philip Reames wrote:
> Duplicate check with previous assert.
Removed.
================
Comment at: lib/Transforms/Scalar/IM.cpp:119
@@ +118,3 @@
+ "Basic block is not a diamond head\n");
+ BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator());
+ BasicBlock *Succ0 = BI->getSuccessor(0);
----------------
Philip Reames wrote:
> Use cast not dyn_cast if it can't fail. Comment repeats throughout.
Done.
================
Comment at: lib/Transforms/Scalar/IM.cpp:134
@@ +133,3 @@
+ BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator());
+ BasicBlock *Succ0 = BI->getSuccessor(0);
+ BasicBlock *Succ1 = BI->getSuccessor(1);
----------------
Philip Reames wrote:
> cast again.
Done.
================
Comment at: lib/Transforms/Scalar/IM.cpp:151
@@ +150,3 @@
+
+/// True when instruction is a hoist barrier for a load
+bool IM::isLoadHoistBarrier(Instruction *Inst) {
----------------
Philip Reames wrote:
> Define "hoist barrier"
Done.
================
Comment at: lib/Transforms/Scalar/IM.cpp:152
@@ +151,3 @@
+/// True when instruction is a hoist barrier for a load
+bool IM::isLoadHoistBarrier(Instruction *Inst) {
+ if (isa<CallInst>(Inst))
----------------
Philip Reames wrote:
> For all of the legality code here, see LICM:canSinkOrHoistInst.
>
> I'm not saying it's an exact duplicate, but it's *very* similar. We should look closely to see if a shared helper function can be extracted.
I had looked at this before. The names are similar, but the context is different. in LICM the parameter is a load and the question is "is the load invariant". In this case the parameter is an instruction and the question is "could this instruction prevent any load from hoisting (above it)".
================
Comment at: lib/Transforms/Scalar/IM.cpp:162
@@ +161,3 @@
+ }
+ // FIXME: do we have to check for memory models?
+ // For example can we hoist above volatile loads?
----------------
Arnold Schwaighofer wrote:
> Philip Reames wrote:
> > Answer: yes. You need to check for fences and load/stores which are ordered w.r.t. the one your moving. (I think the second is actually handled by the isSimple check, but you definitely need the first.)
> >
> > You're also missing invokes.
> The code checks for mayHaveSideEffects which covers fences (=> mayWriteToMemory returns true) and loads that have ordering (=> mayWriteToMemory returns true). Invokes are terminators.
>
> Gerolf's comment is misleading though ...
Arnold's comment is correct. mayHaveSideEffect covered Unordered which covers volatile.
And invokes are caught either by Terminator or mayHaveSideEffect.
I will remove the comment now that we clarified and answered my question. :-)
================
Comment at: lib/Transforms/Scalar/IM.cpp:175
@@ +174,3 @@
+ if (LI->isUsedOutsideOfBlock(LI->getParent()))
+ return I;
+
----------------
Philip Reames wrote:
> When returning null, please just return NULL.
>
> Also, why is this check part of this function? It doesn't seem to match the comment.
OK for NULL.
The code checks for the use of the load, not the load itself. I think the comment could be crisper, though.
================
Comment at: lib/Transforms/Scalar/IM.cpp:181
@@ +180,3 @@
+
+ // Only move loads if they are used in the block.
+ if (isLoadHoistBarrier(Inst))
----------------
Philip Reames wrote:
> I think you mean "are only used in the block".
>
> Why do you need this restriction though?
Yes.
Again simplicity. When the uses are outside the block the live ranges of the original must be equivalent. That is more difficult and expensive to prove.
================
Comment at: lib/Transforms/Scalar/IM.cpp:213
@@ +212,3 @@
+ // Intersect optional metadata.
+ HoistCand->intersectOptionalDataWith(ElseInst);
+
----------------
Arnold Schwaighofer wrote:
> Philip Reames wrote:
> > Shouldn't this be done with the clone?
> He is intersecting metadata from left and right.
>
> Gerolf you will also need to Instruction::dropUnknownMetadata.
Thank you, Arnold!
================
Comment at: lib/Transforms/Scalar/IM.cpp:222
@@ +221,3 @@
+ // Notify AA of the new value.
+ if (isa<LoadInst>(HoistCand))
+ getAliasAnalysis()->copyValue(HoistCand, HoistedInst);
----------------
Philip Reames wrote:
> Don't you need to handle stores, atomics, and a bunch of other memory operations here too? Or are they disallowed and you're missing asserts?
Currently the only instruction hoisted here is a GET.
================
Comment at: lib/Transforms/Scalar/IM.cpp:278
@@ +277,3 @@
+ MergedLoads |= Res;
+ // Don't attempt to hoist above loads that had not been hoisted.
+ if (!Res)
----------------
Philip Reames wrote:
> Why not? What's wrong with doing so?
Simplicity. At this point the code would have to check whether the load in questions aliases with any predecessor load in the same block. The merge algorithm would be no longer linear in #instructions in BB and a bit more complex. I would revisit when the need arises. I guess the action item here is to add a stat counter to check how many opportunities we have in that space.
================
Comment at: lib/Transforms/Scalar/IM.cpp:287
@@ +286,3 @@
+
+/// True when instruction is sink barrier for a store
+bool IM::isStoreSinkBarrier(Instruction *Inst) {
----------------
Philip Reames wrote:
> See previous comments w.r.t. legality.
Ok
================
Comment at: lib/Transforms/Scalar/IM.cpp:358
@@ +357,3 @@
+/// with the GEP instruction computing the store address
+bool IM::sinkStore(BasicBlock *BB, StoreInst *S0, StoreInst *S1) {
+ // Only one definition?
----------------
Philip Reames wrote:
> It looks like you're only sinking gep/store pairs from each basic block right? Why not support a shared gep? Or other similar patterns?
Any extension will come later.
================
Comment at: lib/Transforms/Scalar/IM.cpp:371
@@ +370,3 @@
+ // Intersect optional metadata.
+ S0->intersectOptionalDataWith(S1);
+
----------------
Philip Reames wrote:
> On clone?
See above.
================
Comment at: lib/Transforms/Scalar/IM.cpp:402
@@ +401,3 @@
+/// predecessor block and try to match a store in the second predecessor.
+bool IM::mergeStores(BasicBlock *T) {
+
----------------
Philip Reames wrote:
> This looks like it triggers for some merges which are not diamonds. Intentional? If so, document.
Why? mergeStores in called only for diamonds. The parameters is the footer.
================
Comment at: lib/Transforms/Scalar/IM.cpp:405
@@ +404,3 @@
+ bool MergedStores = false;
+ if (!T)
+ return MergedStores;
----------------
Philip Reames wrote:
> Should this be an assert?
>
> Please just return the constant.
Yes on assert.
================
Comment at: lib/Transforms/Scalar/IM.cpp:441
@@ +440,3 @@
+ else {
+ RBI = Pred0->rbegin();
+ RBE = Pred0->rend();
----------------
Philip Reames wrote:
> You can make this faster in the common case by using a weak reference. See what GVN does here.
>
> Although, can't you simply grab the next instruction before calling sinkStore?
Please point me directly to the code you have in mind.
For stores the code walks in reverse and may sink the GEP also. Possibly at some point we will sink more instructions.
http://reviews.llvm.org/D4096
More information about the llvm-commits
mailing list