From 6d26638c905a796a194fd21482740c9ddc3be788 Mon Sep 17 00:00:00 2001 From: Roman Tereshin Date: Wed, 9 May 2018 21:43:30 +0000 Subject: [PATCH] [GlobalISel][Legalizer] Widening the second src op of shifts bug fix The second source operand of G_SHL, G_ASHR, and G_LSHR must preserve its value as a (small) unsigned integer, therefore its incorrect to widen it in any way but by zero extending it. G_SHL was using G_ANYEXT and G_ASHR - G_SEXT (which is correct for their destination and first source operands, but not the "number of bits to shift" operand). Generally, shifts aren't as similar to regular binary operations as it might seem, for instance, they aren't commutative nor associative and the second source operand usually requires a special treatment. Reviewers: bogner, javed.absar, aivchenk, rovka Reviewed By: bogner Subscribers: igorb, kristof.beyls, llvm-commits Differential Revision: https://reviews.llvm.org/D46413 llvm-svn: 331926 --- .../CodeGen/GlobalISel/LegalizerHelper.cpp | 20 +++++- .../AArch64/GlobalISel/legalize-shift.mir | 21 +++--- .../CodeGen/X86/GlobalISel/ashr-scalar.ll | 6 +- .../X86/GlobalISel/shl-scalar-widening.ll | 67 +++++++++++++++++++ .../test/CodeGen/X86/GlobalISel/shl-scalar.ll | 2 + 5 files changed, 100 insertions(+), 16 deletions(-) create mode 100644 llvm/test/CodeGen/X86/GlobalISel/shl-scalar-widening.ll diff --git a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp index e1b9c6bb6f09..410c3b722705 100644 --- a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp +++ b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp @@ -621,7 +621,6 @@ LegalizerHelper::widenScalar(MachineInstr &MI, unsigned TypeIdx, LLT WideTy) { case TargetOpcode::G_OR: case TargetOpcode::G_XOR: case TargetOpcode::G_SUB: - case TargetOpcode::G_SHL: // Perform operation at larger width (any extension is fine here, high bits // don't affect the result) and then truncate the result back to the // original type. @@ -631,15 +630,32 @@ LegalizerHelper::widenScalar(MachineInstr &MI, unsigned TypeIdx, LLT WideTy) { MIRBuilder.recordInsertion(&MI); return Legalized; + case TargetOpcode::G_SHL: + widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_ANYEXT); + // The "number of bits to shift" operand must preserve its value as an + // unsigned integer: + widenScalarSrc(MI, WideTy, 2, TargetOpcode::G_ZEXT); + widenScalarDst(MI, WideTy); + MIRBuilder.recordInsertion(&MI); + return Legalized; + case TargetOpcode::G_SDIV: case TargetOpcode::G_SREM: - case TargetOpcode::G_ASHR: widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_SEXT); widenScalarSrc(MI, WideTy, 2, TargetOpcode::G_SEXT); widenScalarDst(MI, WideTy); MIRBuilder.recordInsertion(&MI); return Legalized; + case TargetOpcode::G_ASHR: + widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_SEXT); + // The "number of bits to shift" operand must preserve its value as an + // unsigned integer: + widenScalarSrc(MI, WideTy, 2, TargetOpcode::G_ZEXT); + widenScalarDst(MI, WideTy); + MIRBuilder.recordInsertion(&MI); + return Legalized; + case TargetOpcode::G_UDIV: case TargetOpcode::G_UREM: case TargetOpcode::G_LSHR: diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-shift.mir b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-shift.mir index 7aae700f9e8e..781b5d8cde81 100644 --- a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-shift.mir +++ b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-shift.mir @@ -30,26 +30,27 @@ body: | ; CHECK: [[TRUNC:%[0-9]+]]:_(s32) = G_TRUNC [[COPY]](s64) ; CHECK: [[SHL:%[0-9]+]]:_(s32) = G_SHL [[TRUNC]], [[C]] ; CHECK: [[ASHR:%[0-9]+]]:_(s32) = G_ASHR [[SHL]], [[C]] - ; CHECK: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 24 + ; CHECK: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 255 ; CHECK: [[TRUNC1:%[0-9]+]]:_(s32) = G_TRUNC [[COPY1]](s64) - ; CHECK: [[SHL1:%[0-9]+]]:_(s32) = G_SHL [[TRUNC1]], [[C1]] - ; CHECK: [[ASHR1:%[0-9]+]]:_(s32) = G_ASHR [[SHL1]], [[C1]] - ; CHECK: [[ASHR2:%[0-9]+]]:_(s32) = G_ASHR [[ASHR]], [[ASHR1]] - ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[ASHR2]](s32) + ; CHECK: [[AND:%[0-9]+]]:_(s32) = G_AND [[TRUNC1]], [[C1]] + ; CHECK: [[ASHR1:%[0-9]+]]:_(s32) = G_ASHR [[ASHR]], [[AND]] + ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[ASHR1]](s32) ; CHECK: $w0 = COPY [[COPY2]](s32) ; CHECK: [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 255 ; CHECK: [[TRUNC2:%[0-9]+]]:_(s32) = G_TRUNC [[COPY]](s64) - ; CHECK: [[AND:%[0-9]+]]:_(s32) = G_AND [[TRUNC2]], [[C2]] + ; CHECK: [[AND1:%[0-9]+]]:_(s32) = G_AND [[TRUNC2]], [[C2]] ; CHECK: [[C3:%[0-9]+]]:_(s32) = G_CONSTANT i32 255 ; CHECK: [[TRUNC3:%[0-9]+]]:_(s32) = G_TRUNC [[COPY1]](s64) - ; CHECK: [[AND1:%[0-9]+]]:_(s32) = G_AND [[TRUNC3]], [[C3]] - ; CHECK: [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[AND]], [[AND1]] + ; CHECK: [[AND2:%[0-9]+]]:_(s32) = G_AND [[TRUNC3]], [[C3]] + ; CHECK: [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[AND1]], [[AND2]] ; CHECK: [[COPY3:%[0-9]+]]:_(s32) = COPY [[LSHR]](s32) ; CHECK: $w0 = COPY [[COPY3]](s32) ; CHECK: [[TRUNC4:%[0-9]+]]:_(s32) = G_TRUNC [[COPY]](s64) + ; CHECK: [[C4:%[0-9]+]]:_(s32) = G_CONSTANT i32 255 ; CHECK: [[TRUNC5:%[0-9]+]]:_(s32) = G_TRUNC [[COPY1]](s64) - ; CHECK: [[SHL2:%[0-9]+]]:_(s32) = G_SHL [[COPY1]]0, [[COPY1]]1 - ; CHECK: [[COPY4:%[0-9]+]]:_(s32) = COPY [[COPY1]]2(s32) + ; CHECK: [[AND3:%[0-9]+]]:_(s32) = G_AND [[TRUNC5]], [[C4]] + ; CHECK: [[SHL1:%[0-9]+]]:_(s32) = G_SHL [[TRUNC4]], [[AND3]] + ; CHECK: [[COPY4:%[0-9]+]]:_(s32) = COPY [[SHL1]](s32) ; CHECK: $w0 = COPY [[COPY4]](s32) %0(s64) = COPY $x0 %1(s64) = COPY $x1 diff --git a/llvm/test/CodeGen/X86/GlobalISel/ashr-scalar.ll b/llvm/test/CodeGen/X86/GlobalISel/ashr-scalar.ll index 0c71097f0088..29848ade30fd 100644 --- a/llvm/test/CodeGen/X86/GlobalISel/ashr-scalar.ll +++ b/llvm/test/CodeGen/X86/GlobalISel/ashr-scalar.ll @@ -153,8 +153,7 @@ define i1 @test_ashr_i1(i32 %arg1, i32 %arg2) { ; X64: # %bb.0: ; X64-NEXT: shlb $7, %dil ; X64-NEXT: sarb $7, %dil -; X64-NEXT: shlb $7, %sil -; X64-NEXT: sarb $7, %sil +; X64-NEXT: andb $1, %sil ; X64-NEXT: movl %esi, %ecx ; X64-NEXT: sarb %cl, %dil ; X64-NEXT: movl %edi, %eax @@ -171,8 +170,7 @@ define i1 @test_ashr_i1_imm1(i32 %arg1) { ; X64-NEXT: movb $-1, %cl ; X64-NEXT: shlb $7, %dil ; X64-NEXT: sarb $7, %dil -; X64-NEXT: shlb $7, %cl -; X64-NEXT: sarb $7, %cl +; X64-NEXT: andb $1, %cl ; X64-NEXT: sarb %cl, %dil ; X64-NEXT: movl %edi, %eax ; X64-NEXT: retq diff --git a/llvm/test/CodeGen/X86/GlobalISel/shl-scalar-widening.ll b/llvm/test/CodeGen/X86/GlobalISel/shl-scalar-widening.ll new file mode 100644 index 000000000000..02bfa9d09947 --- /dev/null +++ b/llvm/test/CodeGen/X86/GlobalISel/shl-scalar-widening.ll @@ -0,0 +1,67 @@ +; RUN: llc -mtriple=x86_64-linux-gnu -global-isel -verify-machineinstrs < %s -o - | FileCheck %s --check-prefix=X64 + +define i16 @test_shl_i4(i16 %v, i16 %a, i16 %b) { +; Let's say the arguments are the following unsigned +; integers in two’s complement representation: +; +; %v: 77 (0000 0000 0100 1101) +; %a: 74 (0000 0000 0100 1010) +; %b: 72 (0000 0000 0100 1000) + %v.t = trunc i16 %v to i4 ; %v.t: 13 (1101) + %a.t = trunc i16 %a to i4 ; %a.t: 10 (1010) + %b.t = trunc i16 %b to i4 ; %b.t: 8 (1000) + %n.t = add i4 %a.t, %b.t ; %n.t: 2 (0010) + %r.t = shl i4 %v.t, %n.t ; %r.t: 4 (0100) + %r = zext i4 %r.t to i16 +; %r: 4 (0000 0000 0000 0100) + ret i16 %r + +; X64-LABEL: test_shl_i4 +; +; %di: 77 (0000 0000 0100 1101) +; %si: 74 (0000 0000 0100 1010) +; %dx: 72 (0000 0000 0100 1000) +; +; X64: # %bb.0: +; +; X64-NEXT: addb %sil, %dl +; %dx: 146 (0000 0000 1001 0010) +; +; X64-NEXT: andb $15, %dl +; %dx: 2 (0000 0000 0000 0010) +; +; X64-NEXT: movl %edx, %ecx +; %cx: 2 (0000 0000 0000 0010) +; +; X64-NEXT: shlb %cl, %dil +; %di: 52 (0000 0000 0011 0100) +; +; X64-NEXT: andw $15, %di +; %di: 4 (0000 0000 0000 0100) +; +; X64-NEXT: movl %edi, %eax +; %ax: 4 (0000 0000 0000 0100) +; +; X64-NEXT: retq +; +; Let's pretend that legalizing G_SHL by widening its second +; source operand is done via G_ANYEXT rather than G_ZEXT and +; see what happens: +; +; addb %sil, %dl +; %dx: 146 (0000 0000 1001 0010) +; +; movl %edx, %ecx +; %cx: 146 (0000 0000 1001 0010) +; +; shlb %cl, %dil +; %di: 0 (0000 0000 0000 0000) +; +; andw $15, %di +; %di: 0 (0000 0000 0000 0000) +; +; movl %edi, %eax +; %ax: 0 (0000 0000 0000 0000) +; +; retq +} diff --git a/llvm/test/CodeGen/X86/GlobalISel/shl-scalar.ll b/llvm/test/CodeGen/X86/GlobalISel/shl-scalar.ll index f76e02e67996..c0dcde771811 100644 --- a/llvm/test/CodeGen/X86/GlobalISel/shl-scalar.ll +++ b/llvm/test/CodeGen/X86/GlobalISel/shl-scalar.ll @@ -151,6 +151,7 @@ define i8 @test_shl_i8_imm1(i32 %arg1) { define i1 @test_shl_i1(i32 %arg1, i32 %arg2) { ; X64-LABEL: test_shl_i1: ; X64: # %bb.0: +; X64-NEXT: andb $1, %sil ; X64-NEXT: movl %esi, %ecx ; X64-NEXT: shlb %cl, %dil ; X64-NEXT: movl %edi, %eax @@ -165,6 +166,7 @@ define i1 @test_shl_i1_imm1(i32 %arg1) { ; X64-LABEL: test_shl_i1_imm1: ; X64: # %bb.0: ; X64-NEXT: movb $-1, %cl +; X64-NEXT: andb $1, %cl ; X64-NEXT: shlb %cl, %dil ; X64-NEXT: movl %edi, %eax ; X64-NEXT: retq