-
Notifications
You must be signed in to change notification settings - Fork 11k
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.
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
[FastISel][AArch64] Compare Instruction Miscompilation Fix #75993
Changes from 1 commit
5657561
cdc10d8
9652999
44d4231
a495c27
4457315
7017aec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
; RUN: llc --global-isel=false -fast-isel -O0 -mtriple=aarch64-none-none < %s | FileCheck %s | ||
|
||
; Check that the shl instruction did not get folded in together with | ||
; the cmp instruction. It would create a miscompilation | ||
|
||
@A = dso_local global [5 x i8] c"\C8\AA\C8\AA\AA" | ||
@.str = private unnamed_addr constant [13 x i8] c"TRUE BRANCH\0A\00" | ||
@.str.1 = private unnamed_addr constant [14 x i8] c"FALSE BRANCH\0A\00" | ||
|
||
define dso_local i32 @main() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to simplify the test? I think it could
There are some existing tests in llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll, but they don't seem to include any with shift amounts at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm in that case do you think it's more beneficial if I put this test in llvm/test/CodeGen/AArch64/arm64-fast-isel-icmp.ll? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds like it might be a good place for it. Feel free to use the update_llc_test_checks script too to generate the check lines. |
||
|
||
%tmp = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 1) | ||
%tmp2 = load i8, ptr getelementptr inbounds ([5 x i8], ptr @A, i64 0, i64 2) | ||
%op1 = xor i8 %tmp, -49 | ||
%op2 = mul i8 %op1, %op1 | ||
; CHECK-NOT: cmp [[REGS:.*]] #[[SHIFT_VAL:[0-9]+]] | ||
%op3 = shl i8 %op2, 3 | ||
%tmp3 = icmp eq i8 %tmp2, %op3 | ||
br i1 %tmp3, label %_true, label %_false | ||
|
||
_true: | ||
%res = call i32 (ptr, ...) @printf(ptr noundef @.str) | ||
br label %_ret | ||
|
||
_false: | ||
%res2 = call i32 (ptr, ...) @printf(ptr noundef @.str.1) | ||
br label %_ret | ||
|
||
_ret: | ||
ret i32 0 | ||
} | ||
|
||
declare i32 @printf(ptr noundef, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to reach this point now, if NeedExtend==false and ExtendType != AArch64_AM::InvalidShiftExtend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add
ExtendType != AArch64_AM::InvalidShiftExtend
because there is already a check before this nested-if statement in line 1232. Do you think it's still better to add the check?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I meant that if
ExtendType != InvalidShift
then does that mean that NeedExtend is always false? Is it ever possible to get to the emitAddSub_rx(..) line now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah Gotcha. You're right, if
ExtendType != AArch64_AM::InvalidShiftExtend
, NeedExtend is always true. So the logic would never be reached. Maybe it's a better idea to remove that part of the logic all together?