[PATCH] D31782: [CodeGenPrepare]Skip sext promotion if operands for multiple users is detected

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3361-3362
+  if (!ExtOpnd->hasOneUse()) {
+    if (hasMultiUserOperand)
+      *hasMultiUserOperand = true;
+    if (!TLI.isTruncateFree(ExtTy, ExtOpnd->getType()))
----------------
eastig wrote:
> Don't you think a caller of getAction can get this information itself?
> 
> ```
> bool hasMultiUserOperand = false;
> if (Instruction *ExtOpnd = dyn_cast<Instruction>(I->getOperand(0)))
>    hasMultiUserOperand = !ExtOpnd->hasOneUse();
> getAction(I, ...);
> ```
> 
> 'getAction' does not use hasMultiUserOperand nor puts it into Action. This parameter looks like exposing internal calculations of the getAction function. If something is changed in the function and there is no hasOneUse call the function would have to continue calculating hasMultiUserOperand as a part of the contract.
I think it's possible to calculate hasMultiUserOperand in caller as we need this only for address type promotion. 


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4572-4573
     // Get the action to perform the promotion.
-    TypePromotionHelper::Action TPH =
-        TypePromotionHelper::getAction(I, InsertedInsts, *TLI, PromotedInsts);
+    TypePromotionHelper::Action TPH = TypePromotionHelper::getAction(
+        I, InsertedInsts, *TLI, PromotedInsts, hasMultiUserOperand);
     // Check if we can promote.
----------------
eastig wrote:
> Do you assume that the Exts vector always has only one instruction? Otherwise the value of hasMultiUserOperand can be changed from true to false.
I do not assume only one instruction in Exts. After hasMultiUserOperand is initialized as false in  optimizeExt(), hasMultiUserOperand can turn into true only when encountering any operand used by multiple users. I cannot see the case where hasMultiUserOperand turn from true to false ? Please let me know if you see such case. 


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4815
+  // opportunities to fold the extension as part of address modes.
+  if (ATPConsiderable && !hasMultiUserOperand &&
       performAddressTypePromotion(Inst, AllowPromotionWithoutCommonHeader,
----------------
eastig wrote:
> The calculation of hasMultiUserOperand is too far from its use. Is it possible to bring it closer?
> Do we need other calculations/actions if we know an operand has many users before trying to promote?
> What I see we try to promote and discard results if hasMultiUserOperand is true. 'hasMultiUserOperand' can be set up before and transformations. Are there cases when hasMultiUserOperand is set up after transformations?
 We try to promote sext  speculatively which is used in two optimizations : 1) for ExtLoad and 2) address type promotion (initially implemented in aarch64-type-promotion pass) and hasMultiUserOperand is used only for the address type promotion. I think the speculative promotion during the profitability check is just reasonable, and reusing the result of speculative promotions for address type promotion is also reasonable.  

I agree that the calculation of hasMultiUserOperand is too far from its use, but I cannot see any other better way of doing this while reusing the result of speculative promotions.

Quentin, do you have any suggestion ? 


================
Comment at: test/CodeGen/AArch64/arm64-addr-type-promotion.ll:99-100
+  %add = add nsw i32 %j, 1
+  %sext1 = sext i32 %add to i64
+  %arrayidx = getelementptr inbounds %struct.GlobalData, %struct.GlobalData* @global_data, i64 0, i32 0, i64 %sext1
+  store float %s, float* %arrayidx, align 4
----------------
eastig wrote:
> Does this guarantee lowering to multi usage? Is it possible to write a test in MIR?
In this test, I intended to make %add have two users:  1) in %sext1 and 2)  in %j.l.  In such case, we do not allow moving %sext up through %add. 


https://reviews.llvm.org/D31782