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

[Evgeny Astigeevich via Phabricator via llvm-commits]

eastig added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3361-3362
+  if (!ExtOpnd->hasOneUse()) {
+    if (hasMultiUserOperand)
+      *hasMultiUserOperand = true;
+    if (!TLI.isTruncateFree(ExtTy, ExtOpnd->getType()))
----------------
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.


================
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.
----------------
Do you assume that the Exts vector always has only one instruction? Otherwise the value of hasMultiUserOperand can be changed from true to false.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4815
+  // opportunities to fold the extension as part of address modes.
+  if (ATPConsiderable && !hasMultiUserOperand &&
       performAddressTypePromotion(Inst, AllowPromotionWithoutCommonHeader,
----------------
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?


================
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
----------------
Does this guarantee lowering to multi usage? Is it possible to write a test in MIR?


https://reviews.llvm.org/D31782