[PATCH] D130233: [LoopLoadElim] Add stores with matching sizes as load-store candidates

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks! Some additional suggestions inline.



================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:218
+      // 2. one of the pointees is a pointer and
+      //    the other one is an integer or vice versa.
+      if (!CastInst::isBitOrNoopPointerCastable(
----------------
nit: reflow comment, also now that you use `isBitOrNoopPointerCastable` it would probably be easier to say that stored values are bit/pointer-castable.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:448
+    Type *LoadType = Initial->getType();
+    Type *StoreType = Cand.Store->getOperand(0)->getType();
+    auto &DL = Cand.Load->getParent()->getModule()->getDataLayout();
----------------
nit: `getStoredValue()` would be more explicit than `getOperand(0)`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:449
+    Type *StoreType = Cand.Store->getOperand(0)->getType();
+    auto &DL = Cand.Load->getParent()->getModule()->getDataLayout();
+
----------------
Looks like this is only used by the assert. If that's the case, you'll need `(void)DL;` to avoid unused variable warning in release builds.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:455
+    Value *StoreValue;
+
+    if (LoadType != StoreType)
----------------
nit: stray new line?


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:461
+    else
+      StoreValue = Cand.Store->getOperand(0);
+
----------------
Nit: could change to 

```
    Value *StoreValue = Cand.Store->getOperand(0);` 
    if (...)
       StoreValue = CastInst::CreateBitOrPointerCast(StoreValue, LoadType,
                                           "store_forward_cast", Cand.Store);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130233