[LICM] Prevent LICM of ptrtoint and inttoptr when using non-integral pointers

[Il-Capitano]

Non-integral pointers don't have a stable bit representation, i.e. subsequent executions of ptrtoint or inttoptr instructions can create different results, therefore it is invalid to move these instructions outside of a loop when they use non-integral pointer types.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Csanád Hajdú (Il-Capitano)

Changes

Non-integral pointers don't have a stable bit representation, i.e. subsequent executions of ptrtoint or inttoptr instructions can create different results, therefore it is invalid to move these instructions outside of a loop when they use non-integral pointer types.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/LICM.cpp (+15)
• (added) llvm/test/Transforms/LICM/non-integral-pointers.ll (+67)

diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 91ef2b4b7c183..f286d910f932e 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -70,6 +70,7 @@
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/IR/Module.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/PredIteratorCache.h"
 #include "llvm/InitializePasses.h"
@@ -1328,6 +1329,20 @@ bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
           }
       }
     return true;
+  } else if (auto *PTII = dyn_cast<PtrToIntInst>(&I)) {
+    const DataLayout &DL = I.getModule()->getDataLayout();
+    // Non-integral pointers may not have a stable bit representation, therefore
+    // casting them to an integer is not loop invariant.
+    if (DL.isNonIntegralPointerType(PTII->getPointerOperand()->getType())) {
+      return false;
+    }
+  } else if (auto *ITPI = dyn_cast<IntToPtrInst>(&I)) {
+    const DataLayout &DL = I.getModule()->getDataLayout();
+    // Non-integral pointers may not have a stable bit representation, therefore
+    // casting an integer to a non-integral pointer type is not loop invariant.
+    if (DL.isNonIntegralPointerType(ITPI->getType())) {
+      return false;
+    }
   }
 
   assert(!I.mayReadOrWriteMemory() && "unhandled aliasing");
diff --git a/llvm/test/Transforms/LICM/non-integral-pointers.ll b/llvm/test/Transforms/LICM/non-integral-pointers.ll
new file mode 100644
index 0000000000000..01336d7d812b3
--- /dev/null
+++ b/llvm/test/Transforms/LICM/non-integral-pointers.ll
@@ -0,0 +1,67 @@
+; RUN: opt -passes=licm -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-ni:1"
+
+declare void @use_i64(i64 %0)
+declare void @use_p1(ptr addrspace(1) %0)
+declare i1 @cond()
+
+define void @dont_hoist_ptrtoint(ptr addrspace(1) %p) {
+; CHECK-LABEL: @dont_hoist_ptrtoint
+; CHECK-LABEL: loop
+; CHECK:         ptrtoint
+entry:
+  br label %loop
+
+loop:
+  %p.int = ptrtoint ptr addrspace(1) %p to i64
+  call void @use_i64(i64 %p.int)
+  br label %loop
+}
+
+define void @dont_hoist_inttoptr(i64 %p.int) {
+; CHECK-LABEL: @dont_hoist_inttoptr
+; CHECK-LABEL: loop
+; CHECK:         inttoptr
+entry:
+  br label %loop
+
+loop:
+  %p = inttoptr i64 %p.int to ptr addrspace(1)
+  call void @use_p1(ptr addrspace(1) %p)
+  br label %loop
+}
+
+define i64 @dont_sink_ptrtoint(ptr addrspace(1) %p) {
+; CHECK-LABEL: @dont_sink_ptrtoint
+; CHECK-LABEL: loop
+; CHECK:         ptrtoint
+; CHECK-LABEL: exit
+entry:
+  br label %loop
+
+loop:
+  %p.int = ptrtoint ptr addrspace(1) %p to i64
+  %c = call i1 @cond()
+  br i1 %c, label %loop, label %exit
+
+exit:
+  ret i64 %p.int
+}
+
+define ptr addrspace(1) @dont_sink_inttoptr(i64 %p.int) {
+; CHECK-LABEL: @dont_sink_inttoptr
+; CHECK-LABEL: loop
+; CHECK:         inttoptr
+; CHECK-LABEL: exit
+entry:
+  br label %loop
+
+loop:
+  %p = inttoptr i64 %p.int to ptr addrspace(1)
+  %c = call i1 @cond()
+  br i1 %c, label %loop, label %exit
+
+exit:
+  ret ptr addrspace(1) %p
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[DanielKristofKiss]

LGTM, could you rerun clang-format?

[nikic]

I'm pretty sure that this was not an indented consequence of non-integral pointers. Please do not merge this until an expert on non-integral pointers has approved this.

[Il-Capitano]

@preames @arpilipe @annamthomas Pinging for review.

[Il-Capitano]

@preames @arpilipe @annamthomas Ping

[arpilipe]

There was a bit of discussion about LICM in the review that allowed such casts for non-integral pointers.
https://reviews.llvm.org/D104547

At that time, we considered LICM valid because the cast is allowed to produce the same result every time.

What kind of problem are you trying to solve with this restriction?

[Il-Capitano]

In my use case, the result of ptrtoint is used for indexing a GC card table. The issue was that during the loop's execution, the original pointer could get relocated, meaning that a different index would be needed for the card table marking potentially every iteration. However, LICM hoisted the ptrtoint outside of the loop, resulting in the same index being used every iteration, which caused an issue.

Looking at the discussion you linked, and the wording in the Language Reference, I guess my question is what is meant by implementation defined fencing? I assumed that having a call not marked with "gc-leaf-function" (or at least memory(inaccessiblemem: write) to allow it changing the environment state) in the execution path would have sufficed, but LICM (and maybe other optimizations?) doesn't respect that currently. Would this fencing rather be, that the ptrtoint should be replaced by an external function call in order to observe the pointer's bit representation at that particular program state?

[arpilipe]

Here is how we implement fencing: Operations that inspect the bitwise representation of GC pointers are wrapped into function calls. These calls remain opaque to the optimizer until they are inlined. We inline them only after we have inserted explicit relocations using RewriteStatepointsForGC. Once the relocations are explicit, statepoints act as barriers to optimizations.

For example, in your case, if a loop contains a statepoint, the corresponding relocation will create a new def for the value, preventing LICM.

Restricting the hoisting of inttoptr and ptrtoint for non-integral pointers will prevent hoisting in cases where the appropriate fencing has been done. E.g., in our scenario, hoisting is possible after RewriteStatepointsForGC if the loop doesn't contain statepoints, thus can't change the bitwise representation of the pointers.

I'm not sure if this has a significant performance impact, so I don't have a strong preference. However, before we restrict the optimization, you may consider a similar fencing scheme to prevent undesirable transformations.

[Il-Capitano]

Thanks for the explanation, using an opaque function before relocations are finalized seems like the best approach, I'll try that.

[arichardson]

I recently started some work trying to clarify the non-integral pointer semantics. Right now we assume non-integral pointers are both numerically unstable (e.g. copying GC) as well as not just an address. I'd like to split these two properties (Draft PR #105735) and then we'd be able to restrict this to unstable pointers while still allowing it for fat pointers and/or CHERI capabilities.

[Il-Capitano]

I'm closing this PR, as the intended solution seems to be to use a noinline function in place of pointer casts before relocations are finalized. I also realized that if we wanted to make inttoptr and ptrtoint follow similar semantics with non-integral pointers, other passes need to be changed as well, not just LICM, e.g. https://godbolt.org/z/ejqzbos7f.