[PATCH] D45151: [LICM] Hoisting invariant.group loads

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Can we spec `!invariant.group` in a way that lets us always keep the metadata when hoisting?  Right now it isn't clear what happens if its contract is violated, i.e. what the behavior of this program is:

  store i32 0, i32* %ptr, !invariant.group !0
  %x = load i32, i32* %ptr, !invariant.group !0
  store i32 1, i32* %ptr, !invariant.group !0
  %y = load i32, i32* %ptr, !invariant.group !0

though it seems like you're assuming the *load* has UB?  Can we instead say that the second store has UB?  That way we should be able to hoist the load instruction without dropping the metadata.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:542
+  // the metadata.  This is because we don't know yet if it's better to hoist it
+  // and loose metadata, or to keep the metadata counting that we will be able
+  // to merge this load with another outside the loop.
----------------
lose


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1075
       // drop.  It is a compile time optimization, not required for correctness.
-      !isGuaranteedToExecute(I, DT, CurLoop, SafetyInfo))
+      !canKeepMetadata(I, DT, CurLoop, SafetyInfo))
     I.dropUnknownNonDebugMetadata();
----------------
Not sure what this replacement adds -- do you plan to add more stuff to `canKeepMetadata` in the future?


Repository:
  rL LLVM

https://reviews.llvm.org/D45151