Skip to content

[GVN] Turn off ScalarPRE for TokenLike Types #156513

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 2 commits into from
Sep 2, 2025
Merged

[GVN] Turn off ScalarPRE for TokenLike Types #156513

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
863f6c3
[GVN] Turn off ScalarPRE for TokenLike Types
farzonl Sep 2, 2025
e887729
update test case to address pr comments.
farzonl Sep 2, 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
3 changes: 2 additions & 1 deletion llvm/lib/Transforms/Scalar/GVN.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2982,7 +2982,8 @@ bool GVNPass::performScalarPREInsertion(Instruction *Instr, BasicBlock *Pred,
bool GVNPass::performScalarPRE(Instruction *CurInst) {
if (isa<AllocaInst>(CurInst) || CurInst->isTerminator() ||
isa<PHINode>(CurInst) || CurInst->getType()->isVoidTy() ||
CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects())
CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects() ||
CurInst->getType()->isTokenLikeTy())
return false;

// Don't do PRE on compares. The PHI would prevent CodeGenPrepare from
Expand Down
57 changes: 57 additions & 0 deletions llvm/test/Transforms/GVN/PRE/no-phi-translate.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -S -passes=gvn %s | FileCheck %s

; NOTE: when we use a Token like type we should not introduce a phi
Copy link
Member Author

Choose a reason for hiding this comment

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

@bogner i did put this note in

Copy link
Contributor

Choose a reason for hiding this comment

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

This note makes sense - I was saying it might also be good to note that in this case we expect GVN not to do anything at all, which gives some context for reading what the check lines are checking.

So maybe something like "Test that GVN doesn't introduce phis for token like types. NOTE: This implies that GVN won't do anything at all to this input". OTOH maybe the existing comment is clear enough. I'll leave it to you to decide.


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
; CHECK-NOT: phi

Copy link
Member Author

Choose a reason for hiding this comment

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

there can be phi nodes just not on Token like types. This would likely break the phi in the for loop. I think what exist is sufficent because if a phi were to show up later this test would fail and then someone would come read this note.

@Out.str = private unnamed_addr constant [4 x i8] c"Out\00", align 1

define void @CSMain() local_unnamed_addr {
; CHECK-LABEL: define void @CSMain() local_unnamed_addr {
; CHECK-NEXT: [[ENTRY:.*:]]
; CHECK-NEXT: [[TMP0:%.*]] = tail call i32 @llvm.dx.flattened.thread.id.in.group()
; CHECK-NEXT: [[CMP_I1_NOT:%.*]] = icmp eq i32 [[TMP0]], 0
; CHECK-NEXT: br i1 [[CMP_I1_NOT]], label %[[CSMAIN_EXIT:.*]], label %[[FOR_BODY_I_LR_PH:.*]]
; CHECK: [[FOR_BODY_I_LR_PH]]:
; CHECK-NEXT: [[TMP1:%.*]] = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 [[TMP0]], ptr nonnull @Out.str)
; CHECK-NEXT: br label %[[FOR_BODY_I:.*]]
; CHECK: [[FOR_BODY_I]]:
; CHECK-NEXT: [[I_0_I2:%.*]] = phi i32 [ 0, %[[FOR_BODY_I_LR_PH]] ], [ [[INC_I:%.*]], %[[FOR_BODY_I]] ]
; CHECK-NEXT: [[TMP2:%.*]] = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) [[TMP1]], i8 1)
; CHECK-NEXT: [[INC_I]] = add nuw nsw i32 [[I_0_I2]], 1
; CHECK-NEXT: [[EXITCOND:%.*]] = icmp ne i32 [[INC_I]], [[TMP0]]
; CHECK-NEXT: br i1 [[EXITCOND]], label %[[FOR_BODY_I]], label %[[CSMAIN_EXIT_LOOPEXIT:.*]]
; CHECK: [[CSMAIN_EXIT_LOOPEXIT]]:
; CHECK-NEXT: br label %[[CSMAIN_EXIT]]
; CHECK: [[CSMAIN_EXIT]]:
; CHECK-NEXT: [[TMP3:%.*]] = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 [[TMP0]], ptr nonnull @Out.str)
; CHECK-NEXT: [[TMP4:%.*]] = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) [[TMP3]], i8 1)
; CHECK-NEXT: [[TMP5:%.*]] = tail call noundef nonnull align 4 dereferenceable(4) ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) [[TMP3]], i32 0)
; CHECK-NEXT: store i32 [[TMP4]], ptr [[TMP5]], align 4
; CHECK-NEXT: ret void
;
entry:
%0 = tail call i32 @llvm.dx.flattened.thread.id.in.group()
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really need flattened.thread.id.in.group to test this, could just use an i32 argument to the function

%cmp.i1.not = icmp eq i32 %0, 0
br i1 %cmp.i1.not, label %CSMain.exit, label %for.body.i.lr.ph

for.body.i.lr.ph: ; preds = %entry
%1 = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 %0, ptr nonnull @Out.str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please give all of the values names (ie, %buf instead of %1 here) to make tests easier to edit in the future. Also, we don't need to use the overload in the name, might as well let the IR parser do that and just call @llvm.dx.resource.handlefrombinding(...)

br label %for.body.i

for.body.i: ; preds = %for.body.i.lr.ph, %for.body.i
%i.0.i2 = phi i32 [ 0, %for.body.i.lr.ph ], [ %inc.i, %for.body.i ]
%2 = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) %1, i8 1)
%inc.i = add nuw nsw i32 %i.0.i2, 1
%exitcond = icmp ne i32 %inc.i, %0
br i1 %exitcond, label %for.body.i, label %CSMain.exit.loopexit

CSMain.exit.loopexit: ; preds = %for.body.i
br label %CSMain.exit
Copy link
Contributor

Choose a reason for hiding this comment

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

This block is unnecessary


CSMain.exit: ; preds = %CSMain.exit.loopexit, %entry
%3 = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 %0, ptr nonnull @Out.str)
%4 = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) %3, i8 1)
%5 = tail call noundef nonnull align 4 dereferenceable(4) ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) %3, i32 0)
store i32 %4, ptr %5, align 4
ret void
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really need the getpointer and the store here - we can just ret i32 %4 (though also please give %4 a name

}
Loading