[PATCH] D118685: [ArgPromotion] Make implementation offset based

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:130
+          IntIndices.push_back(APInt::getZero(
+              isa<StructType>(TmpTy) ? 32 : OrigOffset.getBitWidth()));
+          TmpTy = NextTy;
----------------
aeubanks wrote:
> would this be nicer if `DataLayout::getIndexTypeSizeInBits()` also handled struct types?
I don't think that would fit the API, as getIndexTypeSizeInBits() works on the pointer type, not the pointed-to/indexed type.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:434
+                                                  const DataLayout &DL) {
+  // Check if the argument itself is marked dereferenceable and aligned.
+  APInt Bytes(64, NeededDerefBytes);
----------------
aeubanks wrote:
> this should be separated out into its own change
Right, I've landed this separately.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:541
 
-        BaseTy = LI->getType();
-      }
+      NeededDerefBytes = std::max(NeededDerefBytes, Off + Size.getFixedValue());
+      NeededAlign = std::max(NeededAlign, LI->getAlign());
----------------
aeubanks wrote:
> the `NeededDerefBytes = ...` is ok to be in this if block because this will only have an effect when we see this offset for the first time because we only support the same type, meaning `Size` will always be the same for a given offset
> 
> this is slightly confusing, perhaps putting it in a separate `!GuaranteedToExecute && Pair.second` block would be clearer
Right, the correctness of this logic depends on only supporting a single type (or at least, a single size). I've extended the comment on the block to explicitly say that.

> this is slightly confusing, perhaps putting it in a separate !GuaranteedToExecute && Pair.second block would be clearer

Not sure I got this, do you mean splitting this into two blocks, one that updates NeededDerefBytes and one that updates NeededAlign?


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:553
+      if (Optional<bool> Res = HandleLoad(LI, /* GuaranteedToExecute */ true))
+        if (!Res)
+          return false;
----------------
aeubanks wrote:
> `!*Res`?
> I don't think we have a test for an unrelated load in the entry block
Nice catch! It looks like this ultimately makes no functional difference, because we still inspect all loads in the loop below. So this code could actually call `HandleLoad()` without checking the return value, from a correctness perspective. Checking the return value just aborts the analysis a bit earlier.

> I don't think we have a test for an unrelated load in the entry block

We have a test for this in basictest.ll at least -- generally, any test with multiple promoted arguments, because the loads for the other arguments will be "unrelated" to the current argument.


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

https://reviews.llvm.org/D118685