[PATCH] D127392: [InstCombine] Combine consecutive loads which are being merged to form a wider load.

[Biplob Mishra via Phabricator via llvm-commits]

bipmis added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3035
+        if (ConstantInt *CI = dyn_cast<ConstantInt>(GEP2->getOperand(1)))
+          Idx2 = CI->getZExtValue();
+
----------------
nikic wrote:
> You are probably looking for `GEP2->accumulateConstantOffsets()` here -- your current code will treat GEPs with multiple indices incorrectly.
Currently we are handling GEP with single indice. With the multiple indices we exit at the check "isa<GetElementPtrInst>(Op2)" . Will keep a look on this if it can be improved.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3077
+        if (!FindAvailableLoadedValue(LI1, LoadBB, BBIt, DefMaxInstsToScan, AA,
+                                      &IsLoadCSE))
+          return nullptr;
----------------
nikic wrote:
> I don't get this check -- you're checking that there is an available value, but not that this value is the previous load. There might still be a clobber in between.
> 
> What you're probably looking for is canInstructionRangeModRef() between the two instructions.
This is basically to check if there are stores or memory accesses b/w the 2 loads which we should consider for before the load merges. The check starts at L2 in reverse and does the memory check till L1.
There are alias tests loadCombine_4consecutive_with_alias* which should give you the idea.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3119
+          Value *Zero = Constant::getNullValue(X->getType());
+          return BinaryOperator::CreateOr(NewOp, Zero);
+        } else
----------------
nikic wrote:
> Uh, what's the point of oring something with zero?
Yeah this is basically cause the visitOr() does an Insert of the returned OR. This may not be needed if we move this to AggressiveInstCombine.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127392/new/

https://reviews.llvm.org/D127392