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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks 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;
----------------
would this be nicer if `DataLayout::getIndexTypeSizeInBits()` also handled struct types?


================
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);
----------------
this should be separated out into its own change


================
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());
----------------
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


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:553
+      if (Optional<bool> Res = HandleLoad(LI, /* GuaranteedToExecute */ true))
+        if (!Res)
+          return false;
----------------
`!*Res`?
I don't think we have a test for an unrelated load in the entry block


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

https://reviews.llvm.org/D118685