[GlobalOpt] Look through non-PointerType constant expressions. #125205

fhahn · 2025-01-31T11:34:28Z

Allow looking through constant expressions. Constant expressions cannot read, modify or leak the global themselves.

I might be missing something, but using analyzeGlobalAux should ensure all (instruction) users that may read, modify or leak the global are checked.

This fixes another regression exposed by
#123518.

Allow looking through constant expressions. Constant expressions cannot read, modify or leak the global themselves. I might be missing something, but using analyzeGlobalAux should ensure all (instruction) users that may read, modify or leak the global are checked. This fixes another regression exposed by llvm#123518.

llvmbot · 2025-01-31T11:34:57Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Allow looking through constant expressions. Constant expressions cannot read, modify or leak the global themselves.

I might be missing something, but using analyzeGlobalAux should ensure all (instruction) users that may read, modify or leak the global are checked.

This fixes another regression exposed by
#123518.

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

3 Files Affected:

(modified) llvm/lib/Transforms/Utils/GlobalStatus.cpp (+12-3)
(modified) llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-ptrtoint-add-constexpr.ll (-2)
(modified) llvm/test/Transforms/GlobalOpt/read-with-constexpr-users.ll (+2-3)

diff --git a/llvm/lib/Transforms/Utils/GlobalStatus.cpp b/llvm/lib/Transforms/Utils/GlobalStatus.cpp
index 0b3016a86e2875..54a72824129b86 100644
--- a/llvm/lib/Transforms/Utils/GlobalStatus.cpp
+++ b/llvm/lib/Transforms/Utils/GlobalStatus.cpp
@@ -78,8 +78,14 @@ static bool analyzeGlobalAux(const Value *V, GlobalStatus &GS,
           return true;
       } else {
         // Ignore dead constant users.
-        if (!isSafeToDestroyConstant(C))
-          return true;
+        if (!isSafeToDestroyConstant(C)) {
+          if (CE) {
+            if (VisitedUsers.insert(CE).second)
+              if (analyzeGlobalAux(CE, GS, VisitedUsers))
+                return true;
+          } else
+            return true;
+        }
       }
     } else if (const Instruction *I = dyn_cast<Instruction>(UR)) {
       if (!GS.HasMultipleAccessingFunctions) {
@@ -166,9 +172,12 @@ static bool analyzeGlobalAux(const Value *V, GlobalStatus &GS,
         if (MTI->getArgOperand(1) == V)
           GS.IsLoaded = true;
       } else if (const MemSetInst *MSI = dyn_cast<MemSetInst>(I)) {
-        assert(MSI->getArgOperand(0) == V && "Memset only takes one pointer!");
+        if (MSI->getArgOperand(0) != V || MSI->getArgOperand(1) == V)
+          return true;
         if (MSI->isVolatile())
           return true;
+        assert(MSI->getArgOperand(0) == V &&
+               "First argument must be the pointer!");
         GS.StoredType = GlobalStatus::Stored;
       } else if (const auto *CB = dyn_cast<CallBase>(I)) {
         if (CB->getIntrinsicID() == Intrinsic::threadlocal_address) {
diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-ptrtoint-add-constexpr.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-ptrtoint-add-constexpr.ll
index 1e87d9266cb380..52bdc4343851ab 100644
--- a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-ptrtoint-add-constexpr.ll
+++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-users-ptrtoint-add-constexpr.ll
@@ -12,8 +12,6 @@ declare i32 @fn1()
 define void @stores_single_use_gep_constexpr() {
 ; CHECK-LABEL: @stores_single_use_gep_constexpr(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store ptr @fn0, ptr @global.20ptr, align 8
-; CHECK-NEXT:    store ptr @fn1, ptr getelementptr inbounds ([[STRUCT_GLOBAL_20PTR:%.*]], ptr @global.20ptr, i64 0, i32 1), align 8
 ; CHECK-NEXT:    ret void
 ;
 entry:
diff --git a/llvm/test/Transforms/GlobalOpt/read-with-constexpr-users.ll b/llvm/test/Transforms/GlobalOpt/read-with-constexpr-users.ll
index 9df31887102f23..1e39cac9722ca5 100644
--- a/llvm/test/Transforms/GlobalOpt/read-with-constexpr-users.ll
+++ b/llvm/test/Transforms/GlobalOpt/read-with-constexpr-users.ll
@@ -5,15 +5,14 @@
 @H = internal global [2 x i64 ] zeroinitializer
 
 ;.
-; CHECK: @G = internal global [2 x i64] zeroinitializer
+; CHECK: @G = internal constant [2 x i64] zeroinitializer
 ; CHECK: @H = internal global [2 x i64] zeroinitializer
 ;.
 define i64 @G_used_by_gep_inttoptr_exprs() {
 ; CHECK-LABEL: define i64 @G_used_by_gep_inttoptr_exprs() local_unnamed_addr {
-; CHECK-NEXT:    [[L:%.*]] = load i64, ptr @G, align 8
 ; CHECK-NEXT:    [[GEP:%.*]] = getelementptr i8, ptr inttoptr (i64 add (i64 ptrtoint (ptr getelementptr inbounds nuw (i8, ptr @G, i64 16) to i64), i64 8) to ptr), i64 1
 ; CHECK-NEXT:    [[C:%.*]] = icmp eq ptr [[GEP]], @G
-; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[C]], i64 [[L]], i64 9
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[C]], i64 0, i64 9
 ; CHECK-NEXT:    ret i64 [[SEL]]
 ;
   %l = load i64, ptr @G, align 8

nikic

I don't think this is safe. We generally cannot analyze past a ptrtoint cast due to its provenance exposure side effect.

Why does your change result in this ptrtoint+add+inttoptr arithmetic being introduced in the first place?

fhahn · 2025-01-31T14:58:36Z

I don't think this is safe. We generally cannot analyze past a ptrtoint cast due to its provenance exposure side effect.

With the patch, we only look through it to determine if the base object may be read or written. Are there any provenance issues that may lead as to miss reads/writes?

Why does your change result in this ptrtoint+add+inttoptr arithmetic being introduced in the first place?

It is not removed/folded in the same way, because we simplify to a compare earlier. Here is roughly what's going on

We start out with

bb:
  %.pre.i.i.i = ptrtoint ptr %17 to i64
  %.pre56.i.i.i = sub i64 ptrtoint (ptr getelementptr inbounds nuw (i8, ptr @g, i64 12560) to i64), %.pre.i.i.i
  %20 = sdiv exact i64 %.pre56.i.i.i, 40
  switch i64 %20, label %34 [
    i64 3, label %21
    i64 2, label %26
    i64 1, label %31
  ]

With the patch, SimplifyCFG converts this to icmps

bb:
  %.pre.i.i.i = ptrtoint ptr %17 to i64
  %.pre56.i.i.i = sub i64 ptrtoint (ptr getelementptr inbounds nuw (i8, ptr @g, i64 12560) to i64), %.pre.i.i.i
  %20 = sdiv exact i64 %.pre56.i.i.i, 40
  %cond = icmp eq i64 %20, 2
  br i1 %cond, label %21, label %28

Then instcombine folds this to

%cond = icmp eq ptr %17, inttoptr (i64 add (i64 ptrtoint (ptr getelementptr inbounds nuw (i8, ptr @_ZN5clang7targetsL7AVRMcusE, i64 12560) to i64), i64 -80) to ptr)

Without the patch, the switch will only get simplified later

.bb:  
  %.pre.i.i.i.i = ptrtoint ptr %.ptr34 to i64
  %.pre56.i.i.i.i = sub i64 ptrtoint (ptr getelementptr inbounds nuw (i8, ptr @g, i64 12560) to i64), %.pre.i.i.i.i
  %17 = sdiv exact i64 %.pre56.i.i.i.i, 40
  %cond = icmp eq i64 %17, 2
  br i1 %cond, label %18, label …

And later instcombine removes the ptrtoint:

 %cond = icmp eq i64 %.02950.i.i.i.i.idx, 12320

efriedma-quic · 2025-02-05T01:44:40Z

My primary concern here would be that the result of the inttoptr might point to a different global. That isn't relevant in the context of an icmp (icmp ignores provenance anyway), but could be relevant in other contexts.

fhahn · 2025-02-05T21:24:53Z

My primary concern here would be that the result of the inttoptr might point to a different global. That isn't relevant in the context of an icmp (icmp ignores provenance anyway), but could be relevant in other contexts.

Are you thinking about something like below?

GlobalStatus determines how a particular global is used. If starting from @G1 we reach an inttoptr that may actually refer to a different global @G2, then we may have marked @G1 as used/stored unnecessarily, but that should be conservatively correct I think? It is also what we are doing at the moment and I don't think with the new code we could accidentally return the status of a different global than we were starting from.

  store ptr inttoptr (i64 add (i64 ptrtoint (ptr @G) to i64, i64 ptrtoint(ptr @G2) to i64)) to ptr, ptr %dst

efriedma-quic · 2025-02-05T22:14:26Z

Say you have something like the following:

if ((uintptr_t)&g1 + 4 == (uintptr_t)&g2) {
  *(int*)((uintptr_t)&g1 + 4) = 10;
}

You have a write to g2, but the store itself doesn't mention g2 at all.

Maybe that's already broken even without your patch, though? I'm suspicious of the way "IsCompared" works.

If comparisons aren't involved, I guess it's safe to recurse through ptrtoint/inttoptr? At least, I can't come up with any way to break it in this context; like you say, it's conservatively correct.

fhahn · 2025-02-06T08:29:34Z

Say you have something like the following:
if ((uintptr_t)&g1 + 4 == (uintptr_t)&g2) {
  *(int*)((uintptr_t)&g1 + 4) = 10;
}
You have a write to g2, but the store itself doesn't mention g2 at all.

Maybe that's already broken even without your patch, though? I'm suspicious of the way "IsCompared" works.

If comparisons aren't involved, I guess it's safe to recurse through ptrtoint/inttoptr? At least, I can't come up with any way to break it in this context; like you say, it's conservatively correct.

Hmm I see. I assumed there's already code to rule out those cases. I checked and the compare based on integers is handled correctly by GlobalOpt due to considering inttoptr/ptrtoint as writing. For constant-expressions, that wouldn't hold as in the current patch. But if the inttoptr/ptrtoint is only applied to one of the globals, then we don't catch it: https://llvm.godbolt.org/z/Mxz8TMPvo

I think we can also run into this issue with non-inbounds GEPs unless I am missing something and that then wouldn't be handled correctly by GlobalOpt: https://llvm.godbolt.org/z/YroTa6Pc6

So with the change we would be incorrect in more cases. Maybe we need to be more careful with compares in general, and if any constant is involved we should make sure that the pointers on both sides are inbounds?

efriedma-quic · 2025-02-07T23:32:07Z

The GEP version is UB due to provenance.

But yes, we probably want need to restrict more optimizations on comparison operators.

fhahn requested review from nikic, jdoerfert, dtcxzyw and efriedma-quic January 31, 2025 11:34

llvmbot added the llvm:transforms label Jan 31, 2025

fhahn changed the title ~~[GlobalOpt] Look through constant expressions.~~ [GlobalOpt] Look through non-PointerType constant expressions. Jan 31, 2025

fhahn mentioned this pull request Jan 31, 2025

[ValueTracking] Support GEPs in matchSimpleRecurrence. #123518

Open

nikic reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GlobalOpt] Look through non-PointerType constant expressions. #125205

[GlobalOpt] Look through non-PointerType constant expressions. #125205

Uh oh!

fhahn commented Jan 31, 2025

Uh oh!

llvmbot commented Jan 31, 2025

Uh oh!

nikic left a comment

Uh oh!

fhahn commented Jan 31, 2025

Uh oh!

efriedma-quic commented Feb 5, 2025

Uh oh!

fhahn commented Feb 5, 2025

Uh oh!

efriedma-quic commented Feb 5, 2025

Uh oh!

fhahn commented Feb 6, 2025

Uh oh!

efriedma-quic commented Feb 7, 2025

Uh oh!

Uh oh!

[GlobalOpt] Look through non-PointerType constant expressions. #125205

Are you sure you want to change the base?

[GlobalOpt] Look through non-PointerType constant expressions. #125205

Uh oh!

Conversation

fhahn commented Jan 31, 2025

Uh oh!

llvmbot commented Jan 31, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn commented Jan 31, 2025

Uh oh!

efriedma-quic commented Feb 5, 2025

Uh oh!

fhahn commented Feb 5, 2025

Uh oh!

efriedma-quic commented Feb 5, 2025

Uh oh!

fhahn commented Feb 6, 2025

Uh oh!

efriedma-quic commented Feb 7, 2025

Uh oh!

Uh oh!