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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 6 inline comments as done.
asbirlea added a comment.

Thanks for the review!



================
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();
----------------
sanjoy wrote:
> 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).
Changed to SmallSetVector.


================
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
----------------
sanjoy wrote:
> 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.
As above, SmallSetVector.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:971
   Value *SomePtr; // Designated pointer to store to.
-  SmallPtrSetImpl<Value *> &PointerMustAliases;
+  const SmallSetVector<Value *, 8> &PointerMustAliases;
   SmallVectorImpl<BasicBlock *> &LoopExitBlocks;
----------------
sanjoy wrote:
> You could also add a `getSetRef()` (like `getArrayRef()`) method to `SetVector` to expose the contained set and pass that into `LoopPromoter` instead of changing `LoopPromoter` itself.
> 
> But this is fine too.
Noted as a future change.


https://reviews.llvm.org/D35439