[InstCombine] Remove redundant alignment assumptions. #123348

fhahn · 2025-01-17T14:23:20Z

Use known bits to remove redundant alignment assumptions.

Libc++ now adds alignment assumptions for std::vector::begin() and std::vector::end(), so I expect we will see quite a bit more assumptions in C++ [1]. Try to clean up some redundant ones to start with.

[1] #108961

llvmbot · 2025-01-17T14:23:55Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Use known bits to remove redundant alignment assumptions.

Libc++ now adds alignment assumptions for std::vector::begin() and std::vector::end(), so I expect we will see quite a bit more assumptions in C++ [1]. Try to clean up some redundant ones to start with.

[1] #108961

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+27-4)
(modified) llvm/test/Transforms/InstCombine/assume-align.ll (+1-4)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 842881156dc67f..ab628e94235913 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -3199,12 +3199,13 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
       // TODO: apply range metadata for range check patterns?
     }
 
-    // Separate storage assumptions apply to the underlying allocations, not any
-    // particular pointer within them. When evaluating the hints for AA purposes
-    // we getUnderlyingObject them; by precomputing the answers here we can
-    // avoid having to do so repeatedly there.
     for (unsigned Idx = 0; Idx < II->getNumOperandBundles(); Idx++) {
       OperandBundleUse OBU = II->getOperandBundleAt(Idx);
+
+      // Separate storage assumptions apply to the underlying allocations, not
+      // any particular pointer within them. When evaluating the hints for AA
+      // purposes we getUnderlyingObject them; by precomputing the answers here
+      // we can avoid having to do so repeatedly there.
       if (OBU.getTagName() == "separate_storage") {
         assert(OBU.Inputs.size() == 2);
         auto MaybeSimplifyHint = [&](const Use &U) {
@@ -3218,6 +3219,28 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
         MaybeSimplifyHint(OBU.Inputs[0]);
         MaybeSimplifyHint(OBU.Inputs[1]);
       }
+
+      // Try to remove redundant alignment assumptions.
+      if (OBU.getTagName() == "align" && OBU.Inputs.size() == 2) {
+        RetainedKnowledge RK = getKnowledgeFromBundle(
+            *cast<AssumeInst>(II), II->bundle_op_info_begin()[Idx]);
+        if (!RK || RK.AttrKind != Attribute::Alignment ||
+            !isPowerOf2_64(RK.ArgValue))
+          continue;
+
+        // Try to get the instruction before the assumption to use as context.
+        Instruction *CtxI = nullptr;
+        if (CtxI && II->getParent()->begin() != II->getIterator())
+          CtxI = II->getPrevNode();
+
+        auto Known = computeKnownBits(RK.WasOn, 1, CtxI);
+        unsigned KnownAlign = 1 << Known.countMinTrailingZeros();
+        if (KnownAlign < RK.ArgValue)
+          continue;
+
+        auto *New = CallBase::removeOperandBundle(II, OBU.getTagID());
+        return New;
+      }
     }
 
     // Convert nonnull assume like:
diff --git a/llvm/test/Transforms/InstCombine/assume-align.ll b/llvm/test/Transforms/InstCombine/assume-align.ll
index f0e02574330861..962ad4f792a0d1 100644
--- a/llvm/test/Transforms/InstCombine/assume-align.ll
+++ b/llvm/test/Transforms/InstCombine/assume-align.ll
@@ -175,7 +175,6 @@ define ptr @dont_fold_assume_align_zero_of_loaded_pointer_into_align_metadata(pt
 define ptr @redundant_assume_align_1(ptr %p) {
 ; CHECK-LABEL: @redundant_assume_align_1(
 ; CHECK-NEXT:    [[P2:%.*]] = load ptr, ptr [[P:%.*]], align 8
-; CHECK-NEXT:    call void @llvm.assume(i1 true) [ "align"(ptr [[P2]], i32 1) ]
 ; CHECK-NEXT:    call void @foo(ptr [[P2]])
 ; CHECK-NEXT:    ret ptr [[P2]]
 ;
@@ -189,7 +188,6 @@ define ptr @redundant_assume_align_1(ptr %p) {
 define ptr @redundant_assume_align_8_via_align_metadata(ptr %p) {
 ; CHECK-LABEL: @redundant_assume_align_8_via_align_metadata(
 ; CHECK-NEXT:    [[P2:%.*]] = load ptr, ptr [[P:%.*]], align 8, !align [[META0:![0-9]+]]
-; CHECK-NEXT:    call void @llvm.assume(i1 true) [ "align"(ptr [[P2]], i32 8) ]
 ; CHECK-NEXT:    call void @foo(ptr [[P2]])
 ; CHECK-NEXT:    ret ptr [[P2]]
 ;
@@ -214,8 +212,7 @@ define ptr @assume_align_16_via_align_metadata(ptr %p) {
 
 define ptr @redundant_assume_align_8_via_align_attribute(ptr align 8 %p) {
 ; CHECK-LABEL: @redundant_assume_align_8_via_align_attribute(
-; CHECK-NEXT:    call void @llvm.assume(i1 true) [ "align"(ptr [[P:%.*]], i32 8) ]
-; CHECK-NEXT:    call void @foo(ptr [[P]])
+; CHECK-NEXT:    call void @foo(ptr [[P:%.*]])
 ; CHECK-NEXT:    ret ptr [[P]]
 ;
   call void @llvm.assume(i1 true) [ "align"(ptr %p, i32 8) ]

goldsteinn · 2025-01-17T18:14:24Z

Can this ever delete unrecoverable information?

nikic · 2025-01-17T21:40:37Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+        // Try to get the instruction before the assumption to use as context.
+        Instruction *CtxI = nullptr;
+        if (CtxI && II->getParent()->begin() != II->getIterator())
+          CtxI = II->getPrevNode();


Hm, I don't get how this works. Don't we scan forward across guaranteed-to-transfer instructions and should find the assume that way? Esp. as we disable the ephemeral value handling for these assumptions.

At the moment, assumes aren't used by IC's computeKnownBits in general because it doesn't pass through AssumptionCache (probably good to improve separately), so this here was intended to just help with using other context-sensitive info.

I've removed it for now.

nikic · 2025-01-17T21:40:43Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+          CtxI = II->getPrevNode();
+
+        auto Known = computeKnownBits(RK.WasOn, 1, CtxI);
+        unsigned KnownAlign = 1 << Known.countMinTrailingZeros();


Can overflow?

Yep, should be fixed in the latest version, although I am not sure how to create a test case where it would actually overflow

fhahn

Can this ever delete unrecoverable information?

The main case where we can lose info is if we derive the alignment from an argument and later inline the function; then the align arg attribute will be gone. Updated to only do this for pointers not derived from an argument.

Without support for GEP recurrences, we would also lose information in some cases when using the alignment of a call result, but that should be fixed with #123518

https://github.com/dtcxzyw/llvm-opt-benchmark/pull/1984/files shows the impact with with both #123518 and this PR

fhahn · 2025-01-21T12:45:51Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+          CtxI = II->getPrevNode();
+
+        auto Known = computeKnownBits(RK.WasOn, 1, CtxI);
+        unsigned KnownAlign = 1 << Known.countMinTrailingZeros();


Yep, should be fixed in the latest version, although I am not sure how to create a test case where it would actually overflow

goldsteinn · 2025-01-21T20:03:26Z

Can this ever delete unrecoverable information?

The main case where we can lose info is if we derive the alignment from an argument and later inline the function; then the align arg attribute will be gone. Updated to only do this for pointers not derived from an argument.

Hmm, maybe we should re-emit alignment assumptions during inlining?

fhahn · 2025-01-21T21:26:21Z

Can this ever delete unrecoverable information?

The main case where we can lose info is if we derive the alignment from an argument and later inline the function; then the align arg attribute will be gone. Updated to only do this for pointers not derived from an argument.

Hmm, maybe we should re-emit alignment assumptions during inlining?

Eventually yes/maybe. The problem is that adding assumptions at the moment can lead to worse results, so before emitting more assumptions I think we should make sure we can clean up ones that don't add any value.

This patch is a small step towards getting better at cleaning things up. @dtcxzyw's https://github.com/dtcxzyw/llvm-opt-benchmark/ is super helpful at surfacing cases where assumptions (or dropping them) causes issues. The current patch together with #123518 is a first step that shouldn't cause any regressions.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

nikic · 2025-02-10T15:26:41Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+        if (!UO || isa<Argument>(UO))
+          continue;
+
+        KnownBits Known = computeKnownBits(RK.WasOn, 0, nullptr);


Comment on why the nullptr context here? Though I think we'd be better off adding an AllowEphemeralValues option to SimplifyQuery and use it as appropriate. We can safely set AllowEphemeralValues=true when inferring alignment and can drop the current special case for align operand bundles.

Added a comment (we don't want to use the assumption we are about to remove).

Though I think we'd be better off adding an AllowEphemeralValues option to SimplifyQuery and use it as appropriate. We can safely set AllowEphemeralValues=true when inferring alignment and can drop the current special case for align operand bundles.
Can be done as separate improvement IIUC, assuming you mean the special case below?

// Allow AllowEphemerals in isValidAssumeForContext, as the CxtI might <<< // be the producer of the pointer in the bundle. At the moment, align // assumptions aren't optimized away. if (RK.WasOn == V && RK.AttrKind == Attribute::Alignment && isPowerOf2_64(RK.ArgValue) && isValidAssumeForContext(I, Q.CxtI, Q.DT, /*AllowEphemerals*/ true)) Known.Zero.setLowBits(Log2_64(RK.ArgValue));

Yes. We set AllowEphemerals=true there on the assumption that known bits won't be used to remove align operand bundles -- which is no longer true.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

llvm/test/Transforms/InstCombine/assume-align.ll

fhahn

ping :)

Rebased on current main, it looks as-is there's only a single case I could spot where alignment gets narrowed/regressed: https://github.com/dtcxzyw/llvm-opt-benchmark/pull/2725/files/50758450294569ef6ac6f41d50dcbc7f189257b1#diff-40a5ffbdcae402ed7681965b1723569e2cafa9ceb214ba4b248aab028c07dbda

This case and more can be further strengthed by #123518.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

fhahn · 2025-09-01T16:34:15Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+        if (!UO || isa<Argument>(UO))
+          continue;
+
+        KnownBits Known = computeKnownBits(RK.WasOn, 0, nullptr);


Added a comment (we don't want to use the assumption we are about to remove).

Though I think we'd be better off adding an AllowEphemeralValues option to SimplifyQuery and use it as appropriate. We can safely set AllowEphemeralValues=true when inferring alignment and can drop the current special case for align operand bundles.
Can be done as separate improvement IIUC, assuming you mean the special case below?

// Allow AllowEphemerals in isValidAssumeForContext, as the CxtI might <<< // be the producer of the pointer in the bundle. At the moment, align // assumptions aren't optimized away. if (RK.WasOn == V && RK.AttrKind == Attribute::Alignment && isPowerOf2_64(RK.ArgValue) && isValidAssumeForContext(I, Q.CxtI, Q.DT, /*AllowEphemerals*/ true)) Known.Zero.setLowBits(Log2_64(RK.ArgValue));

llvm/test/Transforms/InstCombine/assume-align.ll

nikic

The tests seems kind of not really related to the change being implemented?

nikic · 2025-09-01T19:37:58Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Value::MaxAlignmentExponent

Done thanks

nikic · 2025-09-01T19:49:37Z

llvm/test/Transforms/InstCombine/assume-align.ll

Why is this assume being dropped? It does not look redundant.

Missing handling for RK.IRArgValue not being a constant, in which case RK.ArgValue = 1.

Should be fixed now

fhahn

The tests seems kind of not really related to the change being implemented?

Yeah a few were related to folding alignment assumption into loads, which isn't part of the PR. removed

fhahn · 2025-09-02T12:31:42Z

llvm/test/Transforms/InstCombine/assume-align.ll

Missing handling for RK.IRArgValue not being a constant, in which case RK.ArgValue = 1.

Should be fixed now

fhahn · 2025-09-02T12:38:54Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Done thanks

fhahn · 2025-09-08T19:07:55Z

ping :)

dtcxzyw

LG. Thanks.

dtcxzyw · 2025-09-09T15:22:32Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

As we emit a bunch of alignment assumptions, can we turn the tag name align into a tag ID OB_align?

Yep, put up #158078

dtcxzyw · 2025-09-09T15:25:49Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Suggested change

RetainedKnowledge RK = getKnowledgeFromBundle(

*cast<AssumeInst>(II), II->bundle_op_info_begin()[Idx]);

RetainedKnowledge RK = getKnowledgeFromOperandInAssume(

*cast<AssumeInst>(II), Idx);

Updated, thanks, it required adjusting Idx by II->arg_size(0

fhahn · 2025-09-12T12:45:20Z

Failures are

lldb-api.commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
lldb-api.commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py

which seem broken in Linux Precommit CI for the last few days consistently

…3348) Use known bits to remove redundant alignment assumptions. Libc++ now adds alignment assumptions for std::vector::begin() and std::vector::end(), so I expect we will see quite a bit more assumptions in C++ [1]. Try to clean up some redundant ones to start with. [1] llvm/llvm-project#108961 PR: llvm/llvm-project#123348

fhahn requested review from dtcxzyw, goldsteinn and jdoerfert January 17, 2025 14:23

fhahn requested a review from nikic as a code owner January 17, 2025 14:23

llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Jan 17, 2025

nikic reviewed Jan 17, 2025

View reviewed changes

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 18fce21 to be0e9ee Compare January 21, 2025 12:29

fhahn commented Jan 21, 2025

View reviewed changes

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 2100fea to 36756d6 Compare January 21, 2025 21:33

nikic reviewed Feb 10, 2025

View reviewed changes

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 36756d6 to 31df383 Compare August 29, 2025 14:10

fhahn mentioned this pull request Aug 29, 2025

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

zyw-bot mentioned this pull request Aug 29, 2025

pre-commit: PR123348 dtcxzyw/llvm-opt-benchmark#2725

Open

nikic mentioned this pull request Aug 30, 2025

[Clang][x86] Bad codegen for assume_aligned with structured binding packs #156206

Open

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 31df383 to 6cdf9e6 Compare September 1, 2025 16:37

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Sep 1, 2025

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 6cdf9e6 to 44f742f Compare September 1, 2025 16:38

fhahn commented Sep 1, 2025

View reviewed changes

nikic reviewed Sep 1, 2025

View reviewed changes

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 44f742f to 38beef2 Compare September 2, 2025 12:33

fhahn commented Sep 2, 2025

View reviewed changes

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 0bd6728 to 1f6207a Compare September 8, 2025 19:02

dtcxzyw approved these changes Sep 9, 2025

View reviewed changes

[InstCombine] Add tests with foldable and redundant align assumptions.

f4b20e4

fhahn added 5 commits September 11, 2025 11:53

[InstCombine] Remove redundant alignment assumptions.

b507b93

!fixup address comments, update tests

520b119

!fixup address comments, strip some tests

ceb8200

!fixup move to correct place

bf73b92

!fixup use getKnowledgeFromOperandInAssume

697bdf2

fhahn force-pushed the ic-remove-redundant-align-assumes branch from 1f6207a to 697bdf2 Compare September 11, 2025 13:11

Merge branch 'main' into ic-remove-redundant-align-assumes

7794806

fhahn enabled auto-merge (squash) September 12, 2025 11:38

fhahn disabled auto-merge September 12, 2025 12:45

fhahn merged commit 93a1470 into llvm:main Sep 12, 2025
8 of 9 checks passed

fhahn deleted the ic-remove-redundant-align-assumes branch September 12, 2025 12:45

[InstCombine] Remove redundant alignment assumptions. #123348

[InstCombine] Remove redundant alignment assumptions. #123348

Uh oh!

Conversation

fhahn commented Jan 17, 2025

Uh oh!

llvmbot commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goldsteinn commented Jan 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goldsteinn commented Jan 21, 2025

Uh oh!

fhahn commented Jan 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn commented Sep 8, 2025

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn commented Sep 12, 2025

Uh oh!

Uh oh!

Reviewers

llvmbot commented Jan 17, 2025 •

edited

Loading