[Attributor] Don't modify functions outside of the current SCC

[jdoerfert]

Thanks a lot for tracking this down!

LGTM, two nits though: can we include the reproducer as test please, and below.

llvm/include/llvm/Transforms/IPO/Attributor.h

1346

Can you add a doxygen comment here describing when/why we sometimes cannot change a use.

[aeubanks]

I'm not actually sure that this is the right change, it was more of a "does something like this make sense"

Updating the tests, this is doing something wrong since it's replacing a bunch of values with undef. We'd probably need to look at all of the callers to see if they're doing the right thing.

[jdoerfert]

In D100079#2675525, @aeubanks wrote:

I'm not actually sure that this is the right change, it was more of a "does something like this make sense"

Updating the tests, this is doing something wrong since it's replacing a bunch of values with undef. We'd probably need to look at all of the callers to see if they're doing the right thing.

I don't follow, sorry.

[aeubanks]

https://bugs.llvm.org/show_bug.cgi?id=49606 already fixed

llvm/include/llvm/Transforms/IPO/Attributor.h

if (!CI)
       return;
 
     CGUpdater.removeCallSite(*CI);
   }
 
   /// Record that \p U is to be replaces with \p NV after information was
   /// manifested. This also triggers deletion of trivially dead istructions.
   bool changeUseAfterManifest(Use &U, Value &NV) {
+    if (!canChangeUse(U))
+      return false;
     Value *&V = ToBeChangedUses[&U];
     if (V && (V->stripPointerCasts() == NV.stripPointerCasts() ||
               isa_and_nonnull<UndefValue>(V)))
       return false;
     assert((!V || V == &NV || isa<UndefValue>(NV)) &&
            "Use was registered twice for replacement with different values!");
     V = &NV;
     return true;
   }
 
+  bool canChangeUse(Use &U) {
+    if (Functions.empty())
+      return true;
+    if (auto *I = dyn_cast<Instruction>(U.getUser()))
+      return Functions.contains(I->getParent()->getParent());
+    return false;
+  }
+
   /// Helper function to replace all uses of \p V with \p NV. Return true if
   /// there is any change. The flag \p ChangeDroppable indicates if dropppable
   /// uses should be changed too.
   bool changeValueAfterManifest(Value &V, Value &NV,
                                 bool ChangeDroppable = true) {
     bool Changed = false;
     for (auto &U : V.uses())
-      if (ChangeDroppable || !U.getUser()->isDroppable())
+      if ((ChangeDroppable || !U.getUser()->isDroppable()) && canChangeUse(U))
         Changed |= changeUseAfterManifest(U, NV);
 
     return Changed;
   }
 
   /// Record that \p I is to be replaced with `unreachable` after information
   /// was manifested.
   void changeToUnreachableAfterManifest(Instruction *I) {