[PATCH] D28680: [CodeGenPrep] move aarch64-type-promotion to CGP

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:526
 
+  /// \brief Return true if it should be considered for address type promotion.
+  bool shouldConsiderAddressTypePromotion(Instruction *I) const;
----------------
Maybe \return instead of \brief Return


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:527
+  /// \brief Return true if it should be considered for address type promotion.
+  bool shouldConsiderAddressTypePromotion(Instruction *I) const;
+
----------------
A const & would probably be more appropriate.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1096
     CI->eraseFromParent();
+    RemovedExtInsts.insert(CI);
     MadeChange = true;
----------------
CI is not valid anymore at this point.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2824
     /// \pre If !Inst->use_empty(), then New != nullptr
-    InstructionRemover(Instruction *Inst, Value *New = nullptr)
+    InstructionRemover(Instruction *Inst, SetOfInstrs &RemovedExtInsts,
+                       Value *New = nullptr)
----------------
Add a comment for the new arg


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4526
 /// \return true if some promotion happened, false otherwise.
-///
-/// Example:
-/// \code
-/// %ld = load i32* %addr
-/// %add = add nuw i32 %ld, 4
-/// %zext = zext i32 %add to i64
-/// \endcode
-/// =>
-/// \code
-/// %ld = load i32* %addr
-/// %zext = zext i32 %ld to i64
-/// %add = add nuw i64 %zext, 4
-/// \endcode
-/// Thanks to the promotion, we can match zext(load i32*) to i64.
 bool CodeGenPrepare::tryToPromoteExts(
     TypePromotionTransaction &TPT, const SmallVectorImpl<Instruction *> &Exts,
----------------
Why is the example removed here?
Edit: ok I see you moved it.
Maybe have the comment from the other function reference that function (or the other way around).


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4621
 
+bool CodeGenPrepare::mergeSExts(Function &F) {
+  DominatorTree DT(F);
----------------
Add a comment explaining what this method does.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4757
+        // operands. We will promote a SExt used in such complex GEP as we
+        // expect some computation to be merged if they are done on 64 bits.
+        if (I->getNumOperands() > 2) {
----------------
The part mentioning 64 bits is, I believe, not in sync with the code.
This depends on what shouldConsiderAddressTypePromotion returns.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4760
+          IsUsedInComplextGEP = true;
+          break;
+        }
----------------
I wonder how much of this logic applies to other target. What I am saying is that this may be beneficial for AArch64, but is it true for X86 for instance?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4798
+    LastKnownGood = TPT.getRestorationPoint();
+    ATPConsiderable = TTI->shouldConsiderAddressTypePromotion(Inst);
+  }
----------------
What purpose does this serve?
I mean the next rollback will be a noop, right?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4802
+  if (!ATPConsiderable) {
     TPT.rollback(LastKnownGood);
+    return Changed;
----------------
This should only be in the else block, right?

Indeed, if canFormExtLd returns true, the changes are committed and there isn't anything else added to the transaction AFAICT. Thus this call does nothing. 


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4808
+  // 1. SExt is used in a getelementptr with more than 2 operand =>
+  //    likely we can merge some computation if they are done on 64 bits.
+  // 2. The header of the SExt chain is used as a header of other SExt chains =>
----------------
For this condition, same question, how does this apply to something that is not AArch64?


================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:633
 
+bool AArch64TTIImpl::shouldConsiderAddressTypePromotion(Instruction *I) {
+  if (!isa<SExtInst>(I))
----------------
Add comments


https://reviews.llvm.org/D28680