Skip to content
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

Jump to bottom

[DAGCombine] wrong result on -O3 -fuse-ld=lld -flto=full #50663

Closed
llvmbot opened this issue Aug 3, 2021 · 7 comments
Closed

[DAGCombine] wrong result on -O3 -fuse-ld=lld -flto=full #50663

llvmbot opened this issue Aug 3, 2021 · 7 comments
Labels
bugzilla Issues migrated from bugzilla llvm:codegen

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 3, 2021

Bugzilla Link 51321
Resolution FIXED
Resolved on Oct 11, 2021 20:29
Version trunk
OS Windows NT
Blocks #51489
Reporter LLVM Bugzilla Contributor
CC @Arnaud-de-Grandmaison-ARM,@DMG862,@RKSimon,@smithp35,@rotateright,@zhongyunde
Fixed by commit(s) 9790a2a e1e4bf1 49dacda 9b3867e

Extended Description

base on the newest commit 41cedb1, dag combine generate wrong code with the attached test code. we expect the the run result is c=1,h=49, while now is c=0,h=49, and disable the optimization of DAGCombiner::visitANDLike can get expected right result.

clang -mcpu=tsv110 -O3 -fuse-ld=lld -Wall issue.c -save-temps

~/llvm-project-upstream(main) » install/bin/clang -v
Huawei Bisheng Compiler clang version 14.0.0 (ssh://git@gitlab.huawei.com:2222/boole-compiler/llvm-project 41cedb1)

======= test case =========
int printf(const char *, ...);
static int c = 2, d;
short g, h;
int main() {
g ^= 50; // 50
c = 0 || g; // 1
h = g - 1; // 49
d = h & 1; // 1
c &= d; // 1
printf("c=%d,h=%d\n", c, h);
// CHECK-RESULT: c=1,h=49
return 0;
}
@DMG862
Copy link
Mannequin

DMG862 mannequin commented Aug 3, 2021

I originally thought you meant 41cedb1 caused this, but that wouldn't make much sense for a AArch64 bug.

I believe this transform needs a one use check on the Add it is transforming:

llvm-project/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

Line 5136 in ccf1038

if (N0.getOpcode() == ISD::ADD && N1.getOpcode() == ISD::SRL &&

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 4, 2021

Yes, the Operand N0 has multi uses

t46: i32 = add t45, Constant:i32<65535>
$12 = void
(gdb) p N1.dump()
t64: i32 = srl t46, Constant:i64<16>
$13 = void
(gdb) p N0.hasOneUse ()
$14 = false

@rotateright
Copy link
Contributor

Can you post the IR for this example, so I'm sure I have the right test?

Note: if I remove that entire block of code, no regression tests fail. My 'blame' skills aren't good - can anyone tell what the motivating case for this transform looked like? If we don't have one, then we can just delete the code.

@zhongyunde
Copy link
Mannequin

zhongyunde mannequin commented Aug 9, 2021

the Total DAG showed as following, and also welcome some advice on https://reviews.llvm.org/D107692 after your investigation, thanks.

(gdb) p DAG.dump()
SelectionDAG has 36 nodes:
t0: ch = EntryToken
t53: ch = store<(store (s16) into @​g, align 4, !tbaa !​8), trunc to i16> t42:1, t43, GlobalAddress:i64<i16* @​g> 0, undef:i64
t52: ch = store<(store (s16) into @​h, align 4, !tbaa !​8), trunc to i16> t0, t46, GlobalAddress:i64<i16* @​h> 0, undef:i64
t39: ch = TokenFactor t53, t52
t19: ch,glue = callseq_start t39, TargetConstant:i64<0>, TargetConstant:i64<0>
t23: ch,glue = CopyToReg t19, Register:i64 $x0, GlobalAddress:i64<[11 x i8]* @.str> 0
t64: i32 = srl t46, Constant:i64<16>
t14: i32 = and t46, t64
t25: ch,glue = CopyToReg t23, Register:i32 $w1, t14, t23:1
t51: i32 = sign_extend_inreg t46, ValueType:ch:i16
t27: ch,glue = CopyToReg t25, Register:i32 $w2, t51, t25:1
t30: ch,glue = AArch64ISD::CALL t27, TargetGlobalAddress:i64<i32 (i8*, ...)* @​printf> 0, Register:i64 $x0, Register:i32 $w1, Register:i32 $w2, RegisterMask:Untyped, t27:1
t31: ch,glue = callseq_end t30, TargetConstant:i64<0>, TargetConstant:i64<0>, t30:1
t33: i32,ch,glue = CopyFromReg t31, Register:i32 $w0, t31:1
t35: ch,glue = CopyToReg t33:1, Register:i32 $w0, Constant:i32<0>
t42: i32,ch = load<(dereferenceable load (s16) from @​g, align 4, !tbaa !​8), anyext from i16> t0, GlobalAddress:i64<i16* @​g> 0, undef:i64
t43: i32 = xor t42, Constant:i32<50>
t45: i32 = and t43, Constant:i32<65535>
t46: i32 = add t45, Constant:i32<65535>
t36: ch = AArch64ISD::RET_FLAG t35, Register:i32 $w0, t35:1

@rotateright
Copy link
Contributor

Added test:
https://reviews.llvm.org/rG9790a2a72f60bb2caf891658c3

And minimal bug fix:
https://reviews.llvm.org/rGe1e4bf174b09bcd4b25cd624f1

Marking as candidate for 13.0 release.

@tstellar
Copy link
Collaborator

tstellar commented Sep 8, 2021

Merged: 9b3867e

@tstellar
Copy link
Collaborator

mentioned in issue #51489

@tstellar tstellar mentioned this issue Oct 12, 2021
Closed
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla llvm:codegen
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tstellar @rotateright @llvmbot