Skip to content

[LICM] Preserve Disjoint flag on OR when hoisting. #140266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 17, 2025
Merged

[LICM] Preserve Disjoint flag on OR when hoisting. #140266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d88c6d8
[LICM] Add tests for hoisting OR disjoint.
fhahn May 16, 2025
b305b7e
[LICM] Preserve Disjoint flag on OR when hoisting.
fhahn May 16, 2025
44e9e4e
!fixup add comment regarding folding.
fhahn May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions llvm/lib/Transforms/Scalar/LICM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2877,6 +2877,13 @@ static bool hoistBOAssociation(Instruction &I, Loop &L,
if (auto *I = dyn_cast<Instruction>(Inv))
I->setFastMathFlags(Intersect);
NewBO->setFastMathFlags(Intersect);
} else if (Opcode == Instruction::Or) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of code seems like something we should have a helper for. As a follow up, maybe factor something out of other places we do reassociation?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, unless I'm missing something, Reassociate.cpp has the same problem of dropping disjoint flags. Note that there is the existing OverflowTracking helper, which might serve as a starting place for the API.

bool Disjoint = cast<PossiblyDisjointInst>(BO)->isDisjoint() &&
cast<PossiblyDisjointInst>(BO0)->isDisjoint();
// If `Inv` was not constant-folded, a new Instruction has been created.
if (auto *I = dyn_cast<PossiblyDisjointInst>(Inv))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast is fragile; it only works under certain assumptions about what the IRBuilder will not fold.

Maybe use IRBuilder<NoFolder>, so you can just cast<>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this uses the default ConstantFolder, which should be fine, as we either get a constant or the required. OR instruction. This is similar to the code for the other cases above.

Is there any particlar case you are thinking where this would be too fragile? I can imagine that we would have to be more careful if the folder could fold the op to an existing other IR value, in which case we may end up setting the disjoint flags on a different OR than we intended. But I don't think that can be the case with the ConstantFolder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not actually wrong with ConstantFolder, I guess, but the interaction is pretty subtle. I've seen similar-looking code cause real issues.

Maybe it's enough to spell out that it's using ConstantFolder, and why it's getting used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added the same comment as we have above for the ADD/MUL case.

I->setIsDisjoint(Disjoint);
cast<PossiblyDisjointInst>(NewBO)->setIsDisjoint(Disjoint);
}

BO->replaceAllUsesWith(NewBO);
Expand Down
63 changes: 63 additions & 0 deletions llvm/test/Transforms/LICM/hoist-binop.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,69 @@ loop:
br label %loop
}

; Trivially hoist or disjoint.
define void @or_all_disjoint(i64 %c1, i64 %c2) {
; CHECK-LABEL: @or_all_disjoint(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[INVARIANT_OP:%.*]] = or disjoint i64 [[C1:%.*]], [[C2:%.*]]
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[INDEX_NEXT_REASS:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[INDEX_NEXT_REASS]] = or disjoint i64 [[INDEX]], [[INVARIANT_OP]]
; CHECK-NEXT: br label [[LOOP]]
;
entry:
br label %loop

loop:
%index = phi i64 [ 0, %entry ], [ %index.next, %loop ]
%step.add = or disjoint i64 %index, %c1
%index.next = or disjoint i64 %c2, %step.add
br label %loop
}

; Trivially hoist or, disjoint on first or only .
define void @or_disjoint_on_first_or_only(i64 %c1, i64 %c2) {
; CHECK-LABEL: @or_disjoint_on_first_or_only(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[INVARIANT_OP:%.*]] = or i64 [[C1:%.*]], [[C2:%.*]]
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[INDEX_NEXT_REASS:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[INDEX_NEXT_REASS]] = or i64 [[INDEX]], [[INVARIANT_OP]]
; CHECK-NEXT: br label [[LOOP]]
;
entry:
br label %loop

loop:
%index = phi i64 [ 0, %entry ], [ %index.next, %loop ]
%step.add = or i64 %index, %c1
%index.next = or disjoint i64 %c2, %step.add
br label %loop
}

; Trivially hoist or, disjoint on second or only .
define void @or_disjoint_on_second_or_only(i64 %c1, i64 %c2) {
; CHECK-LABEL: @or_disjoint_on_second_or_only(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[INVARIANT_OP:%.*]] = or i64 [[C1:%.*]], [[C2:%.*]]
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[INDEX_NEXT_REASS:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[INDEX_NEXT_REASS]] = or i64 [[INDEX]], [[INVARIANT_OP]]
; CHECK-NEXT: br label [[LOOP]]
;
entry:
br label %loop

loop:
%index = phi i64 [ 0, %entry ], [ %index.next, %loop ]
%step.add = or disjoint i64 %index, %c1
%index.next = or i64 %c2, %step.add
br label %loop
}

; Trivially hoist xor.
define void @xor(i64 %c1, i64 %c2) {
; CHECK-LABEL: @xor(
Expand Down