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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1096
     CI->eraseFromParent();
+    RemovedExtInsts.insert(CI);
     MadeChange = true;
----------------
qcolombet wrote:
> CI is not valid anymore at this point.
No need to add CI in RemovedExtInsts because none of extension removed here is considered in optimizeExt().


================
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) {
----------------
qcolombet wrote:
> The part mentioning 64 bits is, I believe, not in sync with the code.
> This depends on what shouldConsiderAddressTypePromotion returns.
Yes, I move this comment in AArch64TTIImpl::shouldConsiderAddressTypePromotion().


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4760
+          IsUsedInComplextGEP = true;
+          break;
+        }
----------------
qcolombet wrote:
> 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?
I believe this should target specific. So I move this into AArch64TTIImpl by adding another parameter (AllowPromotionWithoutCommonHeader) decided in shouldConsiderAddressTypePromotion().


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4798
+    LastKnownGood = TPT.getRestorationPoint();
+    ATPConsiderable = TTI->shouldConsiderAddressTypePromotion(Inst);
+  }
----------------
qcolombet wrote:
> What purpose does this serve?
> I mean the next rollback will be a noop, right?
Yes, look like this is useless. I removed it. If  canFormExtLd is true, return from this function (optimizeExt) instead.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4802
+  if (!ATPConsiderable) {
     TPT.rollback(LastKnownGood);
+    return Changed;
----------------
qcolombet wrote:
> 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. 
In case case canFormExtLd returns false, we should check ATPConsiderable and if ATPConsiderable is false rollback should happen. 


================
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 =>
----------------
qcolombet wrote:
> For this condition, same question, how does this apply to something that is not AArch64?
This should be target specific. So this should be mentioned in  AArch64TTIImpl .


https://reviews.llvm.org/D28680