CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119138

resistor · 2024-12-08T18:18:51Z

No description provided.

llvmbot · 2024-12-08T18:19:24Z

@llvm/pr-subscribers-llvm-transforms

Author: Owen Anderson (resistor)

Changes

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

2 Files Affected:

(modified) llvm/lib/Transforms/Utils/CallPromotionUtils.cpp (+7-3)
(added) llvm/test/Transforms/Inline/promote-call-bitwidth.ll (+47)

diff --git a/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp b/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp
index 17cba2e642a19a..5c8246c14b3882 100644
--- a/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp
+++ b/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp
@@ -692,14 +692,17 @@ bool llvm::tryPromoteCall(CallBase &CB) {
   if (!VTableEntryLoad)
     return false; // Not a vtable entry load.
   Value *VTableEntryPtr = VTableEntryLoad->getPointerOperand();
-  APInt VTableOffset(DL.getTypeSizeInBits(VTableEntryPtr->getType()), 0);
+  APInt VTableOffset(DL.getIndexSizeInBits(
+                         VTableEntryPtr->getType()->getPointerAddressSpace()),
+                     0);
   Value *VTableBasePtr = VTableEntryPtr->stripAndAccumulateConstantOffsets(
       DL, VTableOffset, /* AllowNonInbounds */ true);
   LoadInst *VTablePtrLoad = dyn_cast<LoadInst>(VTableBasePtr);
   if (!VTablePtrLoad)
     return false; // Not a vtable load.
   Value *Object = VTablePtrLoad->getPointerOperand();
-  APInt ObjectOffset(DL.getTypeSizeInBits(Object->getType()), 0);
+  APInt ObjectOffset(
+      DL.getIndexSizeInBits(Object->getType()->getPointerAddressSpace()), 0);
   Value *ObjectBase = Object->stripAndAccumulateConstantOffsets(
       DL, ObjectOffset, /* AllowNonInbounds */ true);
   if (!(isa<AllocaInst>(ObjectBase) && ObjectOffset == 0))
@@ -712,7 +715,8 @@ bool llvm::tryPromoteCall(CallBase &CB) {
       VTablePtrLoad, VTablePtrLoad->getParent(), BBI, 0, nullptr, nullptr);
   if (!VTablePtr)
     return false; // No vtable found.
-  APInt VTableOffsetGVBase(DL.getTypeSizeInBits(VTablePtr->getType()), 0);
+  APInt VTableOffsetGVBase(
+      DL.getIndexSizeInBits(VTablePtr->getType()->getPointerAddressSpace()), 0);
   Value *VTableGVBase = VTablePtr->stripAndAccumulateConstantOffsets(
       DL, VTableOffsetGVBase, /* AllowNonInbounds */ true);
   GlobalVariable *GV = dyn_cast<GlobalVariable>(VTableGVBase);
diff --git a/llvm/test/Transforms/Inline/promote-call-bitwidth.ll b/llvm/test/Transforms/Inline/promote-call-bitwidth.ll
new file mode 100644
index 00000000000000..f0e9f7aec00d6b
--- /dev/null
+++ b/llvm/test/Transforms/Inline/promote-call-bitwidth.ll
@@ -0,0 +1,47 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
+; RUN: opt -S -passes=inline < %s | FileCheck %s
+target datalayout = "e-m:e-p200:128:128:128:64-p:64:64-i64:64-i128:128-n64-S128-A200-P200-G200"
+target triple = "riscv64-unknown-freebsd13.0"
+
+define void @test(ptr addrspace(200) %arg1, ptr addrspace(200) %arg2) local_unnamed_addr addrspace(200) {
+; CHECK-LABEL: define {{[^@]+}}@test
+; CHECK-SAME: (ptr addrspace(200) [[ARG1:%.*]], ptr addrspace(200) [[ARG2:%.*]]) local_unnamed_addr addrspace(200) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load ptr addrspace(200), ptr addrspace(200) [[ARG2]], align 16
+; CHECK-NEXT:    call addrspace(200) void [[TMP0]](ptr addrspace(200) [[ARG1]])
+; CHECK-NEXT:    ret void
+;
+entry:
+  call void @call_fnptr(ptr addrspace(200) %arg1, ptr addrspace(200) %arg2)
+  ret void
+}
+
+define internal void @call_fnptr(ptr addrspace(200) %this, ptr addrspace(200) %arg) unnamed_addr addrspace(200) align 2 {
+entry:
+  %0 = load ptr addrspace(200), ptr addrspace(200) %arg, align 16
+  call void %0(ptr addrspace(200) %this)
+  ret void
+}
+
+define void @test2(ptr addrspace(200) %this) local_unnamed_addr addrspace(200) {
+; CHECK-LABEL: define {{[^@]+}}@test2
+; CHECK-SAME: (ptr addrspace(200) [[THIS:%.*]]) local_unnamed_addr addrspace(200) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[VTABLE_I:%.*]] = load ptr addrspace(200), ptr addrspace(200) [[THIS]], align 16
+; CHECK-NEXT:    [[FN_I:%.*]] = load ptr addrspace(200), ptr addrspace(200) [[VTABLE_I]], align 16
+; CHECK-NEXT:    call addrspace(200) void [[FN_I]](ptr addrspace(200) [[THIS]])
+; CHECK-NEXT:    ret void
+;
+entry:
+  call void @call_via_vtable(ptr addrspace(200) %this)
+  ret void
+}
+
+define internal void @call_via_vtable(ptr addrspace(200) %this) unnamed_addr addrspace(200) {
+entry:
+  %0 = bitcast ptr addrspace(200) %this to ptr addrspace(200)
+  %vtable = load ptr addrspace(200), ptr addrspace(200) %0, align 16
+  %fn = load ptr addrspace(200), ptr addrspace(200) %vtable, align 16
+  call void %fn(ptr addrspace(200) %this)
+  ret void
+}

llvm/lib/Transforms/Utils/CallPromotionUtils.cpp

llvm/test/Transforms/Inline/promote-call-bitwidth.ll

resistor · 2024-12-09T23:27:45Z

Tagging @jrtc27 and @arichardson as this is a CHERI-derived change

arichardson

Looks like this is my downstream commit CTSRD-CHERI/llvm-project@fb9fa37, would be nice if you could add Co-authored-by:.

Thanks for working on upstreaming these changes!

llvm/test/Transforms/Inline/promote-call-bitwidth.ll

arichardson

LGTM with suggested changes.

resistor · 2024-12-10T00:48:12Z

Looks like this is my downstream commit CTSRD-CHERI/llvm-project@fb9fa37, would be nice if you could add Co-authored-by:.

Sure. Is that just written into the commit message, or do I need to do something special to make git recognize it?

arichardson · 2024-12-10T00:50:38Z

Looks like this is my downstream commit CTSRD-CHERI/llvm-project@fb9fa37, would be nice if you could add Co-authored-by:.

Sure. Is that just written into the commit message, or do I need to do something special to make git recognize it?

Just needs to be a separate line in the commit message for github to recognize it. I believe I was still at Cambridge Uni back then so adding
Co-authored-by: Alexander Richardson <[email protected]> would be correct.

llvm/test/Transforms/Inline/promote-call-bitwidth.ll

resistor · 2024-12-10T01:22:19Z

Looks like this is my downstream commit CTSRD-CHERI/llvm-project@fb9fa37, would be nice if you could add Co-authored-by:.

Sure. Is that just written into the commit message, or do I need to do something special to make git recognize it?

Just needs to be a separate line in the commit message for github to recognize it. I believe I was still at Cambridge Uni back then so adding Co-authored-by: Alexander Richardson <[email protected]> would be correct.

Done

nikic

LGTM

…width of pointer offsets. Co-authored-by: Alexander Richardson <[email protected]>

…the bit width of pointer offsets. (#119138)" Reverting due to ASAN bootstrap failures. This reverts commit 4027e2f.

resistor · 2024-12-11T00:23:38Z

I just reverted this due to ASAN bootstrap failures. https://lab.llvm.org/buildbot/#/builders/52/builds/4390
I am investigating these failures ASAP.

jrtc27 · 2024-12-11T00:57:41Z

llvm/lib/Transforms/Utils/CallPromotionUtils.cpp

@@ -712,7 +712,7 @@ bool llvm::tryPromoteCall(CallBase &CB) {
      VTablePtrLoad, VTablePtrLoad->getParent(), BBI, 0, nullptr, nullptr);
  if (!VTablePtr)
    return false; // No vtable found.
-  APInt VTableOffsetGVBase(DL.getTypeSizeInBits(VTablePtr->getType()), 0);


Presumably FindAvailableLoadedValue can return a pointer-sized integer (really, anything that CastInst::isBitOrNoopPointerCastable) if you end up storing a uintptr_t that you then load and call. If so, stripAndAccumulateConstantOffsets returns its input for non-pointers and then the dyn_cast will definitely give null.

(Obviously not true on CHERI for various reasons)

The other instances are fine because they're using the pointer operands of loads so by definition are pointers, whereas this one is some random other Value that is known to be the same bit pattern as the value loaded here.

So probably line 713 needs to gain a type check. Maybe you could cast back instead, though I'd worry about provenance due to peeking through inttoptr(ptrtoint(...)), and AFAICT the former would make the new code behave like the old code.

Thanks. I'm doing a bootstrap build now to try to extract a test case. I'll put up a new PR with the test and the fix once I have that.

…width of pointer offsets. This reapplies llvm#119138 with a defensive fix for the assertion failure when building libcxx. Unfortunately the failure does not reproduce on my machine, so I am not able to extract a test case. The key insight for the fix comes from jrtc, who observes that `VTablePtr` may, in fact, not be a pointer on return from `FindAvailableLoadedValue`. This is easily worked around, because we are guaranteed that the vtable being manipulated is the same one accessed via `VTablePtrLoad` earlier, and thus can obtain the address space from there. Co-authored-by: Alexander Richardson <[email protected]>

…width of pointer offsets. This reapplies llvm#119138 with a defensive fix for the assertion failure when building libcxx. Unfortunately the failure does not reproduce on my machine, so I am not able to extract a test case. The key insight for the fix comes from Jessica Clarke, who observes that `VTablePtr` may, in fact, not be a pointer on return from `FindAvailableLoadedValue`. Co-authored-by: Alexander Richardson <[email protected]>

…width of pointer offsets. (#119483) This reapplies #119138 with a defensive fix for the assertion failure when building libcxx. Unfortunately the failure does not reproduce on my machine, so I am not able to extract a test case. The key insight for the fix comes from Jessica Clarke, who observes that `VTablePtr` may, in fact, not be a pointer on return from `FindAvailableLoadedValue`. Co-authored-by: Alexander Richardson <[email protected]>

llvmbot added the llvm:transforms label Dec 8, 2024

nikic reviewed Dec 9, 2024

View reviewed changes

llvm/lib/Transforms/Utils/CallPromotionUtils.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/Inline/promote-call-bitwidth.ll Outdated Show resolved Hide resolved

resistor requested review from jrtc27 and arichardson December 9, 2024 23:27

arichardson reviewed Dec 10, 2024

View reviewed changes

llvm/test/Transforms/Inline/promote-call-bitwidth.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/Inline/promote-call-bitwidth.ll Show resolved Hide resolved

arichardson approved these changes Dec 10, 2024

View reviewed changes

jrtc27 reviewed Dec 10, 2024

View reviewed changes

llvm/test/Transforms/Inline/promote-call-bitwidth.ll Outdated Show resolved Hide resolved

resistor mentioned this pull request Dec 10, 2024

DataLayout: Fix latent issues with getMaxIndexSizeInBits #118740

Merged

resistor force-pushed the callpromo branch from c644975 to 07e602a Compare December 10, 2024 01:21

arichardson approved these changes Dec 10, 2024

View reviewed changes

nikic approved these changes Dec 10, 2024

View reviewed changes

CallPromotionUtils: Correctly use IndexSize when determining the bit …

4cf53b5

…width of pointer offsets. Co-authored-by: Alexander Richardson <[email protected]>

resistor force-pushed the callpromo branch from 07e602a to 4cf53b5 Compare December 10, 2024 22:51

resistor merged commit 4027e2f into llvm:main Dec 10, 2024
8 checks passed

resistor added a commit that referenced this pull request Dec 11, 2024

Revert "CallPromotionUtils: Correctly use IndexSize when determining …

9b6bb83

…the bit width of pointer offsets. (#119138)" Reverting due to ASAN bootstrap failures. This reverts commit 4027e2f.

resistor mentioned this pull request Dec 11, 2024

CodeGen: Eliminate dynamic relocations in the register superclass tables. #119122

Merged

jrtc27 reviewed Dec 11, 2024

View reviewed changes

resistor mentioned this pull request Dec 11, 2024

CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119138

CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119138

resistor commented Dec 8, 2024

llvmbot commented Dec 8, 2024

resistor commented Dec 9, 2024

arichardson left a comment

arichardson left a comment

resistor commented Dec 10, 2024

arichardson commented Dec 10, 2024

resistor commented Dec 10, 2024

nikic left a comment

resistor commented Dec 11, 2024

jrtc27 Dec 11, 2024

resistor Dec 11, 2024

CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119138

CallPromotionUtils: Correctly use IndexSize when determining the bit width of pointer offsets. #119138

Conversation

resistor commented Dec 8, 2024

llvmbot commented Dec 8, 2024

resistor commented Dec 9, 2024

arichardson left a comment

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

resistor commented Dec 10, 2024

arichardson commented Dec 10, 2024

resistor commented Dec 10, 2024

nikic left a comment

Choose a reason for hiding this comment

resistor commented Dec 11, 2024

jrtc27 Dec 11, 2024

Choose a reason for hiding this comment

resistor Dec 11, 2024

Choose a reason for hiding this comment