[PATCH] D63582: [LICM & MSSA] Limit unsafe sinking and hoisting.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!

I realize that the original test-case is difficult to minimize and reproduce, so having a 'fails before, succeeds now' test isn't easy to make, nor would it be easy to reason about if it breaks. Still, can we please have a quick lit test that asserts e.g.

  for i
    load a[i]
    store a[i]

doesn't have the load sunk?



================
Comment at: lib/Transforms/Scalar/LICM.cpp:380
+                                 LicmMssaOptCap, LicmMssaNoAccForPromotionCap,
+                                 true};
   if (L->hasDedicatedExits())
----------------
nit: please add /*IsSink=*/


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1238
                 return false;
+              // Disable hoising past potentially interfering loads. Optimized
+              // Uses may point to an access outside the loop, as getClobbering
----------------
s/hoising/hoisting/


================
Comment at: lib/Transforms/Scalar/LICM.cpp:2303
+      for (const auto &MA : *Accesses) {
+        if (dyn_cast<MemoryPhi>(&MA))
+          continue;
----------------
s/dyn_cast/isa

also, `isa<MemoryDef>(X)` implies `!isa<MemoryUse>(X)`, so is this check needed?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:2305
+          continue;
+        else if (const auto *MD = dyn_cast<MemoryDef>(&MA)) {
+          if (MU->getBlock() != MD->getBlock() ||
----------------
nit: if the check is needed, no `else` after an `if` that `continue`s, please


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63582