[PATCH] D15047: [Mips64] Fix extension of 32-bit integer types.
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 27 11:14:29 PST 2015
dsanders added a subscriber: vkalintiris.
dsanders added a comment.
Could you point me at the original problem you're trying to solve? The new node appears to be doing the same job as AssertZExt and I'm fairly certain this is headed into a bigger problem we encountered back in spring.
The summary of the larger bug is that MIPS64 lacks a 32-bit comparison instruction (the same opcode performs a 64-bit comparison) but the code generator isn't properly aware of this. As a result, the code generator expects the hardware to ignore bits 32-63 but they actually affect the result. This bug gets exposed as we increase our reliance on the code generator getting it right by removing the redundant extends.
@vkalintiris has been working on a fix to the larger problem (http://reviews.llvm.org/D10970 and a second part to handle the operands). Once both are committed we should be able to safely remove any redundant sign extends that remain.
================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:2909-2910
@@ -2906,3 +2908,4 @@
// size. Extract the value and insert any appropriate assertions regarding
- // sign/zero extension.
+ // sign/zero extension. In N32/N64 ABI unsigned 32-bit integers are
+ // represented in a 64-bit register as sign-extended value.
switch (VA.getLocInfo()) {
----------------
Our calling convention doesn't really matter at this point since clang is responsible for emitting appropriate signext/zeroext for the given ABI. We just need to follow what the IR says.
================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:2927-2933
@@ -2923,4 +2926,9 @@
case CCValAssign::ZExt:
- Val = DAG.getNode(ISD::AssertZext, DL, LocVT, Val, DAG.getValueType(ValVT));
- Val = DAG.getNode(ISD::TRUNCATE, DL, ValVT, Val);
+ // We don't need to sign extend values smaller than 32bit.
+ if (ABI.AreGprs64bit() && ArgVT.getSimpleVT() < MVT::i32)
+ Val = DAG.getNode(MipsISD::TruncNoExt, DL, ValVT, Val);
+ else {
+ Val = DAG.getNode(ISD::AssertZext, DL, LocVT, Val, DAG.getValueType(ValVT));
+ Val = DAG.getNode(ISD::TRUNCATE, DL, ValVT, Val);
+ }
break;
----------------
AssertZext already eliminates any extends the the TRUNCATE would otherwise introduce. I suspect something else in the DAG is causing the sign extends you're trying to remove.
================
Comment at: test/CodeGen/Mips/divrem.ll:76-186
@@ -75,113 +75,113 @@
; ACC32: mfhi $2
; ACC64: mfhi $2
; ALL: .end srem1
%rem = srem i32 %a0, %a1
ret i32 %rem
}
-define i32 @udiv1(i32 zeroext %a0, i32 zeroext %a1) nounwind readnone {
+define i32 @udiv1(i32 signext %a0, i32 signext %a1) nounwind readnone {
entry:
; ALL-LABEL: udiv1:
; ACC32: divu $zero, $4, $5
; ACC32-TRAP: teq $5, $zero, 7
; ACC64: divu $zero, $4, $5
; ACC64-TRAP: teq $5, $zero, 7
; GPR32: divu $2, $4, $5
; GPR32-TRAP: teq $5, $zero, 7
; GPR64: divu $2, $4, $5
; GPR64-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ACC32: mflo $2
; ACC64: mflo $2
; ALL: .end udiv1
%div = udiv i32 %a0, %a1
ret i32 %div
}
-define i32 @urem1(i32 zeroext %a0, i32 zeroext %a1) nounwind readnone {
+define i32 @urem1(i32 signext %a0, i32 signext %a1) nounwind readnone {
entry:
; ALL-LABEL: urem1:
; ACC32: divu $zero, $4, $5
; ACC32-TRAP: teq $5, $zero, 7
; ACC64: divu $zero, $4, $5
; ACC64-TRAP: teq $5, $zero, 7
; GPR32: modu $2, $4, $5
; GPR32-TRAP: teq $5, $zero, 7
; GPR64: modu $2, $4, $5
; GPR64-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ACC32: mfhi $2
; ACC64: mfhi $2
; ALL: .end urem1
%rem = urem i32 %a0, %a1
ret i32 %rem
}
define i32 @sdivrem1(i32 signext %a0, i32 signext %a1, i32* nocapture %r) nounwind {
entry:
; ALL-LABEL: sdivrem1:
; ACC32: div $zero, $4, $5
; ACC32-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ACC32: mflo $2
; ACC32: mfhi $[[R0:[0-9]+]]
; ACC32: sw $[[R0]], 0(${{[0-9]+}})
; ACC64: div $zero, $4, $5
; ACC64-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ACC64: mflo $2
; ACC64: mfhi $[[R0:[0-9]+]]
; ACC64: sw $[[R0]], 0(${{[0-9]+}})
; GPR32: mod $[[R0:[0-9]+]], $4, $5
; GPR32-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; GPR32: sw $[[R0]], 0(${{[0-9]+}})
; GPR32-DAG: div $2, $4, $5
; GPR32-TRAP: teq $5, $zero, 7
; GPR64: mod $[[R0:[0-9]+]], $4, $5
; GPR64-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; GPR64: sw $[[R0]], 0(${{[0-9]+}})
; GPR64-DAG: div $2, $4, $5
; GPR64-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ALL: .end sdivrem1
%rem = srem i32 %a0, %a1
store i32 %rem, i32* %r, align 4
%div = sdiv i32 %a0, %a1
ret i32 %div
}
-define i32 @udivrem1(i32 zeroext %a0, i32 zeroext %a1, i32* nocapture %r) nounwind {
+define i32 @udivrem1(i32 signext %a0, i32 signext %a1, i32* nocapture %r) nounwind {
entry:
; ALL-LABEL: udivrem1:
; ACC32: divu $zero, $4, $5
; ACC32-TRAP: teq $5, $zero, 7
; NOCHECK-NOT: teq
; ACC32: mflo $2
; ACC32: mfhi $[[R0:[0-9]+]]
; ACC32: sw $[[R0]], 0(${{[0-9]+}})
----------------
Please leave these as zeroext. They use zeroext to avoid noise around the interesting generated code and there's no need to match the standard ABI in this kind of test.
================
Comment at: test/CodeGen/Mips/mips64-sign-extend.ll:6
@@ +5,3 @@
+entry:
+; CHECK: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+ %shr = lshr i64 %var, 32
----------------
This will match any i32->i64 sign extend in the output file, even in different functions. You need some CHECK-LABEL's to restrict the scope of the matching.
================
Comment at: test/CodeGen/Mips/mips64-sign-extend.ll:31
@@ +30,3 @@
+entry:
+; CHECK: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+ %add = add i32 %var, 5
----------------
As above, this could match in any function.
================
Comment at: test/CodeGen/Mips/mips64-sign-extend.ll:38-45
@@ +37,10 @@
+entry:
+; CHECK-NOT: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+ %add = add i16 %var, 5
+ ret i16 %add
+}
+
+define i8 @foo4(i8 zeroext %var) #0 {
+entry:
+; CHECK-NOT: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+ %add = add i8 %var, 5
----------------
These CHECK-NOT's don't do what you expect if the earlier CHECK's match in an unexpected function.
================
Comment at: test/CodeGen/Mips/octeon_popcnt.ll:24
@@ -23,3 +23,3 @@
-define i32 @cnt32(i32 zeroext %x) nounwind readnone {
+define i32 @cnt32(i32 signext %x) nounwind readnone {
%cnt = tail call i32 @llvm.ctpop.i32(i32 %x)
----------------
Please leave these as zeroext. They use zeroext to avoid noise around the interesting generated code and there's no need to match the standard ABI in this kind of test.
Repository:
rL LLVM
http://reviews.llvm.org/D15047
More information about the llvm-commits
mailing list