Add utility function for collecting pointer operands of memory-access instructions.

[PeddleSpam]

This is required for #161375.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Leon Clark (PeddleSpam)

Changes

This is required for #161375.

---

Full diff: https://github.com/llvm/llvm-project/pull/172616.diff

2 Files Affected:

• (modified) llvm/include/llvm/IR/Instructions.h (+32)
• (modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+10-28)

diff --git a/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h
index 80e9cb5bd806b..003ff5b8b3a81 100644
--- a/llvm/include/llvm/IR/Instructions.h
+++ b/llvm/include/llvm/IR/Instructions.h
@@ -5196,6 +5196,38 @@ inline void setAtomicSyncScopeID(Instruction *I, SyncScope::ID SSID) {
     llvm_unreachable("unhandled atomic operation");
 }
 
+/// A helper function that returns the pointer operands of a memory-access
+/// instruction.
+inline std::optional<SmallVector<const Value *, 2u>>
+getMemAccessOperands(const Instruction *I) {
+  if (const LoadInst *LI = dyn_cast<LoadInst>(I))
+    return {{LI->getPointerOperand()}};
+  if (const StoreInst *SI = dyn_cast<StoreInst>(I))
+    return {{SI->getPointerOperand()}};
+  if (const VAArgInst *VAAI = dyn_cast<VAArgInst>(I))
+    return {{VAAI->getPointerOperand()}};
+  if (const AtomicCmpXchgInst *CXI = dyn_cast<AtomicCmpXchgInst>(I))
+    return {{CXI->getPointerOperand()}};
+  if (const AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I))
+    return {{RMWI->getPointerOperand()}};
+  if (const auto *Call = dyn_cast<CallBase>(I)) {
+    if (Call->doesNotAccessMemory())
+      return std::nullopt;
+
+    SmallVector<const Value *, 2u> PtrArgs;
+    for (Value *Arg : Call->args()) {
+      if (!Arg->getType()->isPointerTy())
+        continue;
+
+      PtrArgs.push_back(Arg);
+    }
+
+    return PtrArgs;
+  }
+
+  return std::nullopt;
+}
+
 //===----------------------------------------------------------------------===//
 //                              FreezeInst Class
 //===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index f49fbf8807bac..8dc0326bde05f 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1189,19 +1189,9 @@ static void AddAliasScopeMetadata(CallBase &CB, ValueToValueMapTy &VMap,
         continue;
 
       bool IsArgMemOnlyCall = false, IsFuncCall = false;
-      SmallVector<const Value *, 2> PtrArgs;
-
-      if (const LoadInst *LI = dyn_cast<LoadInst>(I))
-        PtrArgs.push_back(LI->getPointerOperand());
-      else if (const StoreInst *SI = dyn_cast<StoreInst>(I))
-        PtrArgs.push_back(SI->getPointerOperand());
-      else if (const VAArgInst *VAAI = dyn_cast<VAArgInst>(I))
-        PtrArgs.push_back(VAAI->getPointerOperand());
-      else if (const AtomicCmpXchgInst *CXI = dyn_cast<AtomicCmpXchgInst>(I))
-        PtrArgs.push_back(CXI->getPointerOperand());
-      else if (const AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I))
-        PtrArgs.push_back(RMWI->getPointerOperand());
-      else if (const auto *Call = dyn_cast<CallBase>(I)) {
+      std::optional<SmallVector<const Value *, 2u>> PtrArgs = getMemAccessOperands(I);
+
+      if (const auto *Call = dyn_cast<CallBase>(I)) {
         // If we know that the call does not access memory, then we'll still
         // know that about the inlined clone of this call site, and we don't
         // need to add metadata.
@@ -1219,23 +1209,13 @@ static void AddAliasScopeMetadata(CallBase &CB, ValueToValueMapTy &VMap,
           if (ME.onlyAccessesArgPointees())
             IsArgMemOnlyCall = true;
         }
-
-        for (Value *Arg : Call->args()) {
-          // Only care about pointer arguments. If a noalias argument is
-          // accessed through a non-pointer argument, it must be captured
-          // first (e.g. via ptrtoint), and we protect against captures below.
-          if (!Arg->getType()->isPointerTy())
-            continue;
-
-          PtrArgs.push_back(Arg);
-        }
       }
 
       // If we found no pointers, then this instruction is not suitable for
       // pairing with an instruction to receive aliasing metadata.
       // However, if this is a call, this we might just alias with none of the
       // noalias arguments.
-      if (PtrArgs.empty() && !IsFuncCall)
+      if (!PtrArgs && !IsFuncCall)
         continue;
 
       // It is possible that there is only one underlying object, but you
@@ -1244,11 +1224,13 @@ static void AddAliasScopeMetadata(CallBase &CB, ValueToValueMapTy &VMap,
       SmallPtrSet<const Value *, 4> ObjSet;
       SmallVector<Metadata *, 4> Scopes, NoAliases;
 
-      for (const Value *V : PtrArgs) {
-        SmallVector<const Value *, 4> Objects;
-        getUnderlyingObjects(V, Objects, /* LI = */ nullptr);
+      if (PtrArgs) {
+        for (const Value *V : *PtrArgs) {
+          SmallVector<const Value *, 4> Objects;
+          getUnderlyingObjects(V, Objects, /* LI = */ nullptr);
 
-        ObjSet.insert_range(Objects);
+          ObjSet.insert_range(Objects);
+        }
       }
 
       // Figure out if we're derived from anything that is not a noalias

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions h,cpp -- llvm/include/llvm/IR/Instructions.h llvm/lib/Transforms/Utils/InlineFunction.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 8dc0326bd..0a1e59970 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1189,7 +1189,8 @@ static void AddAliasScopeMetadata(CallBase &CB, ValueToValueMapTy &VMap,
         continue;
 
       bool IsArgMemOnlyCall = false, IsFuncCall = false;
-      std::optional<SmallVector<const Value *, 2u>> PtrArgs = getMemAccessOperands(I);
+      std::optional<SmallVector<const Value *, 2u>> PtrArgs =
+          getMemAccessOperands(I);
 
       if (const auto *Call = dyn_cast<CallBase>(I)) {
         // If we know that the call does not access memory, then we'll still

[nikic]

I don't think you should be conflating the basic load/store with the call case. The call case requires special handling depending on context.

[nikic]

I think using MemoryLocation::getOrNone() instead would work?

[PeddleSpam]

I think using MemoryLocation::getOrNone() instead would work?

Thanks, this will work nicely 😊