[PATCH] D35439: Make promoteLoopAccessesToScalars independent of AliasSet [NFC]

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:304
+        assert(!AS.empty() &&
+          "Must alias set should have at least one pointer element in it!");
+
----------------
The indent looks off here (clang-format should fix it).


================
Comment at: lib/Transforms/Scalar/LICM.cpp:307
+        SmallPtrSet<Value *, 4> PointerMustAliases;
+        for (const auto &ASI : AS) {
+          PointerMustAliases.insert(ASI.getValue());
----------------
LLVM style avoids `{` for single statement loops and if statements.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1037
+    SmallPtrSetImpl<Value *> &PointerMustAliases,
+    SmallVectorImpl<BasicBlock *> &ExitBlocks,
     SmallVectorImpl<Instruction *> &InsertPts, PredIteratorCache &PIC,
----------------
Does this modify `PointerMustAliases`?  If not, make this a const reference.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1047
 
-  // We can promote this alias set if it has a store, if it is a "Must" alias
-  // set, if the pointer is loop invariant, and if we are not eliminating any
-  // volatile loads or stores.
-  if (AS.isForwardingAliasSet() || !AS.isMod() || !AS.isMustAlias() ||
-      AS.isVolatile() || !CurLoop->isLoopInvariant(AS.begin()->getValue()))
-    return false;
-
-  assert(!AS.empty() &&
-         "Must alias set should have at least one pointer element in it!");
-
-  Value *SomePtr = AS.begin()->getValue();
+  Value *SomePtr = *PointerMustAliases.begin();
   BasicBlock *Preheader = CurLoop->getLoopPreheader();
----------------
I think this can introduce non-determinism since SmallPtrSet does not have a deterministic order.  You'll need some other way to pick a canonical address (just passing it in explicitly is fine).


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1135
   // different sizes.  While we are at it, collect alignment and AA info.
-  for (const auto &ASI : AS) {
-    Value *ASIV = ASI.getValue();
-    PointerMustAliases.insert(ASIV);
-
+  for (Value * ASIV : PointerMustAliases) {
     // Check that all of the pointers in the alias set have the same type.  We
----------------
Whitespace is off (but again, clang-format will fix this).

And the iteration order is not deterministic here either and this can produce non-deterministic output (via LoopUses).  I'm not a 100% sure about the non-determinism, but we should use a stable iteration order unless we can prove that the output will be deterministic.


https://reviews.llvm.org/D35439