[GlobalOpt] Do not fold away addrspacecasts which may be runtime operations

[resistor]

Specifically in the context of the once-stored transformation, GlobalOpt would strip
all pointer casts unconditionally, even though addrspacecasts might be runtime operations.
This manifested particularly on CHERI targets.

This patch was inspired by an existing change in CHERI LLVM (CHERIoT-Platform/llvm-project@91afa60), but has been reimplemented with updated conventions, and a testcase constructed from scratch.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Owen Anderson (resistor)

Changes

Specifically in the context of the once-stored transformation, GlobalOpt would strip
all pointer casts unconditionally, even though addrspacecasts might be runtime operations.
This manifested particularly on CHERI targets.

This patch was inspired by an existing change in CHERI LLVM (CHERIoT-Platform/llvm-project@91afa60), but has been reimplemented with updated conventions, and a testcase constructed from scratch.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+3-2)
• (added) llvm/test/Transforms/GlobalOpt/stored-once-addrspacecast.ll (+31)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index bdda4980c1005..89a811e3f74d5 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1133,8 +1133,9 @@ static bool
 optimizeOnceStoredGlobal(GlobalVariable *GV, Value *StoredOnceVal,
                          const DataLayout &DL,
                          function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
-  // Ignore no-op GEPs and bitcasts.
-  StoredOnceVal = StoredOnceVal->stripPointerCasts();
+  // Ignore no-op GEPs and bitcasts, but not addrspacecasts that may be runtime
+  // operations.
+  StoredOnceVal = StoredOnceVal->stripPointerCastsSameRepresentation();
 
   // If we are dealing with a pointer global that is initialized to null and
   // only has one (non-null) value stored into it, then we can optimize any
diff --git a/llvm/test/Transforms/GlobalOpt/stored-once-addrspacecast.ll b/llvm/test/Transforms/GlobalOpt/stored-once-addrspacecast.ll
new file mode 100644
index 0000000000000..cb2fcce51a7ae
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/stored-once-addrspacecast.ll
@@ -0,0 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals
+; RUN: opt -passes=globalopt < %s -S | FileCheck %s
+
+; Test that we do not fold away addresscasts when optimizing once-stored
+; globals, as these may be runtime operations.
+
+%T = type { ptr }
+
+@0 = internal global ptr null
+@1 = internal addrspace(1) global i32 0
+
+;.
+; CHECK: @[[GLOB0:[0-9]+]] = internal unnamed_addr global ptr null
+; CHECK: @[[GLOB1:[0-9]+]] = internal addrspace(1) global i32 0
+;.
+define void @a() {
+; CHECK-LABEL: @a(
+; CHECK-NEXT:    [[TMP1:%.*]] = addrspacecast ptr addrspace(1) @[[GLOB1]] to ptr
+; CHECK-NEXT:    store ptr [[TMP1]], ptr @[[GLOB0]], align 8
+; CHECK-NEXT:    [[TMP2:%.*]] = load ptr, ptr @[[GLOB0]], align 8
+; CHECK-NEXT:    [[TMP3:%.*]] = load atomic i64, ptr [[TMP2]] acquire, align 8
+; CHECK-NEXT:    ret void
+;
+  %1 = addrspacecast ptr addrspace(1) @1 to ptr
+  store ptr %1, ptr @0, align 8
+  %2 = load ptr, ptr @0, align 8
+  %3 = load atomic i64, ptr %2 acquire, align 8
+  ret void
+}
+
+declare ptr @_Znwm(i64)

[resistor]

Tagging @davidchisnall as the author of the original change, and @jrtc27 and @arichardson for a CHERI-related change.

[jrtc27]

Isn't this only relevant to hybrid CHERI?

[resistor]

Isn't this only relevant to hybrid CHERI?

Yes, I believe it only creates a practical problem for hybrid CHERI.

The existing transformation is still incorrect per LangRef, and the test does not depend on anything CHERI-specific. The condition for blocking the transformation could be tightened when both address spaces are integral pointers (guaranteeing the casts are reversible), but that seems to be in flux per #105735 If that's an issue, I'm happy to wait on this change until that PR is sorted out, and then implement the tighter condition.

[nikic]

LGTM

[resistor]

Squashing and rebasing