[llvm] r323690 - [X86] Avoid using high register trick for test instruction
Eric Liu via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 30 06:16:34 PST 2018
Hi Amaury,
I am seeing crash in llc caused by this commit. A repro is attached. Please
run llc (with assertion on) to reproduce the crash. I'll revert the commit
for now. Let me know if you need more information from me.
```
./bin/llc repro.ll
/home/ioeric/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:469: unsigned
int llvm::InstrEmitter::ConstrainForSubReg(unsigned int, unsigned int,
llvm::MVT, const llvm::DebugLoc &): Assertion `RC && "No legal register
class for VT supports that SubIdx"' failed.
#0 0x0000000002fc9134 PrintStackTraceSignalHandler(void*)
(./bin/llc+0x2fc9134)
#1 0x0000000002fc9496 SignalHandler(int) (./bin/llc+0x2fc9496)
#2 0x00007fa2af1690c0 __restore_rt
(/lib/x86_64-linux-gnu/libpthread.so.0+0x110c0)
#3 0x00007fa2adcfefcf gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x32fcf)
#4 0x00007fa2add003fa abort (/lib/x86_64-linux-gnu/libc.so.6+0x343fa)
#5 0x00007fa2adcf7e37 (/lib/x86_64-linux-gnu/libc.so.6+0x2be37)
#6 0x00007fa2adcf7ee2 (/lib/x86_64-linux-gnu/libc.so.6+0x2bee2)
#7 0x0000000002dc3883 llvm::InstrEmitter::ConstrainForSubReg(unsigned int,
unsigned int, llvm::MVT, llvm::DebugLoc const&) (./bin/llc+0x2dc3883)
#8 0x0000000002dc3ebe llvm::InstrEmitter::EmitSubregNode(llvm::SDNode*,
llvm::DenseMap<llvm::SDValue, unsigned int,
llvm::DenseMapInfo<llvm::SDValue>,
llvm::detail::DenseMapPair<llvm::SDValue, unsigned int> >&, bool, bool)
(./bin/llc+0x2dc3ebe)
#9 0x0000000002dd7c29
llvm::ScheduleDAGSDNodes::EmitSchedule(llvm::MachineInstrBundleIterator<llvm::MachineInstr,
false>&) (./bin/llc+0x2dd7c29)
#10 0x0000000002e81fe2 llvm::SelectionDAGISel::CodeGenAndEmitDAG()
(./bin/llc+0x2e81fe2)
#11 0x0000000002e7ecd8
llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&)
(./bin/llc+0x2e7ecd8)
#12 0x0000000002e7adc4
llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&)
(./bin/llc+0x2e7adc4)
#13 0x00000000022f69b1 (anonymous
namespace)::X86DAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&)
(./bin/llc+0x22f69b1)
#14 0x000000000268b5e8
llvm::MachineFunctionPass::runOnFunction(llvm::Function&)
(./bin/llc+0x268b5e8)
#15 0x00000000029a775f llvm::FPPassManager::runOnFunction(llvm::Function&)
(./bin/llc+0x29a775f)
#16 0x00000000029a7a23 llvm::FPPassManager::runOnModule(llvm::Module&)
(./bin/llc+0x29a7a23)
#17 0x00000000029a7f49 llvm::legacy::PassManagerImpl::run(llvm::Module&)
(./bin/llc+0x29a7f49)
#18 0x00000000017f18d1 compileModule(char**, llvm::LLVMContext&)
(./bin/llc+0x17f18d1)
#19 0x00000000017ef0eb main (./bin/llc+0x17ef0eb)
#20 0x00007fa2adcec2b1 __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x202b1)
#21 0x00000000017ee02a _start (./bin/llc+0x17ee02a)
Stack dump:
0. Program arguments: ./bin/llc repro.ll
1. Running pass 'Function Pass Manager' on module 'repro.ll'.
2. Running pass 'X86 DAG->DAG Instruction Selection' on function
'@str_copy'
Aborted
```
Regards,
Eric
On Mon, Jan 29, 2018 at 9:56 PM Amaury Sechet via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: deadalnix
> Date: Mon Jan 29 12:54:33 2018
> New Revision: 323690
>
> URL: http://llvm.org/viewvc/llvm-project?rev=323690&view=rev
> Log:
> [X86] Avoid using high register trick for test instruction
>
> Summary:
> It seems it's main effect is to create addition copies when values are inr
> register that do not support this trick, which increase register pressure
> and makes the code bigger.
>
> The main noteworthy regression I was able to observe was pattern of the
> type (setcc (trunc (and X, C)), 0) where C is such as it would benefit from
> the hi register trick. To prevent this, a new pattern is added to
> materialize such pattern using a 32 bits test. This has the added benefit
> of working with any constant that is materializable as a 32bits immediate,
> not just the ones that can leverage the high register trick, as
> demonstrated by the test case in test-shrink.ll using the constant 2049 .
>
> Reviewers: craig.topper, niravd, spatel, hfinkel
>
> Subscribers: llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D42646
>
> Modified:
> llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp
> llvm/trunk/lib/Target/X86/X86InstrArithmetic.td
> llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
> llvm/trunk/lib/Target/X86/X86MacroFusion.cpp
> llvm/trunk/test/CodeGen/X86/test-shrink.ll
> llvm/trunk/test/CodeGen/X86/testb-je-fusion.ll
> llvm/trunk/test/CodeGen/X86/vastart-defs-eflags.ll
>
> Modified: llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp Mon Jan 29 12:54:33 2018
> @@ -3073,67 +3073,33 @@ void X86DAGToDAGISel::Select(SDNode *Nod
> return;
> }
>
> - // For example, "testl %eax, $2048" to "testb %ah, $8".
> - if (isShiftedUInt<8, 8>(Mask) &&
> - (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
> - // Shift the immediate right by 8 bits.
> - SDValue ShiftedImm = CurDAG->getTargetConstant(Mask >> 8, dl,
> MVT::i8);
> - SDValue Reg = N0.getOperand(0);
> -
> - // Extract the h-register.
> - SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_8bit_hi,
> dl,
> - MVT::i8, Reg);
> -
> - // Emit a testb. The EXTRACT_SUBREG becomes a COPY that can only
> - // target GR8_NOREX registers, so make sure the register class is
> - // forced.
> - SDNode *NewNode = CurDAG->getMachineNode(X86::TEST8ri_NOREX, dl,
> - MVT::i32, Subreg,
> ShiftedImm);
> - // Replace SUB|CMP with TEST, since SUB has two outputs while
> TEST has
> - // one, do not call ReplaceAllUsesWith.
> - ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
> - SDValue(NewNode, 0));
> - CurDAG->RemoveDeadNode(Node);
> - return;
> - }
> -
> - // For example, "testl %eax, $32776" to "testw %ax, $32776".
> - // NOTE: We only want to form TESTW instructions if optimizing for
> - // min size. Otherwise we only save one byte and possibly get a
> length
> - // changing prefix penalty in the decoders.
> - if (OptForMinSize && isUInt<16>(Mask) && N0.getValueType() !=
> MVT::i16 &&
> - (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
> - SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i16);
> - SDValue Reg = N0.getOperand(0);
> -
> - // Extract the 16-bit subregister.
> - SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_16bit,
> dl,
> - MVT::i16, Reg);
> -
> - // Emit a testw.
> - SDNode *NewNode = CurDAG->getMachineNode(X86::TEST16ri, dl,
> MVT::i32,
> - Subreg, Imm);
> - // Replace SUB|CMP with TEST, since SUB has two outputs while
> TEST has
> - // one, do not call ReplaceAllUsesWith.
> - ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
> - SDValue(NewNode, 0));
> - CurDAG->RemoveDeadNode(Node);
> - return;
> - }
> -
> // For example, "testq %rax, $268468232" to "testl %eax,
> $268468232".
> - if (isUInt<32>(Mask) && N0.getValueType() == MVT::i64 &&
> + if (isUInt<32>(Mask) &&
> (!(Mask & 0x80000000) || hasNoSignedComparisonUses(Node))) {
> - SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i32);
> + MVT VT = MVT::i32;
> + int SubRegOp = X86::sub_32bit;
> + unsigned Op = X86::TEST32ri;
> +
> + // For example, "testl %eax, $32776" to "testw %ax, $32776".
> + // NOTE: We only want to form TESTW instructions if optimizing for
> + // min size. Otherwise we only save one byte and possibly get a
> length
> + // changing prefix penalty in the decoders.
> + if (OptForMinSize && isUInt<16>(Mask) &&
> + (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
> + VT = MVT::i16;
> + SubRegOp = X86::sub_16bit;
> + Op = X86::TEST16ri;
> + }
> +
> + SDValue Imm = CurDAG->getTargetConstant(Mask, dl, VT);
> SDValue Reg = N0.getOperand(0);
>
> - // Extract the 32-bit subregister.
> - SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_32bit,
> dl,
> - MVT::i32, Reg);
> + // Extract the subregister if necessary.
> + if (N0.getValueType() != VT)
> + Reg = CurDAG->getTargetExtractSubreg(SubRegOp, dl, VT, Reg);
>
> - // Emit a testl.
> - SDNode *NewNode = CurDAG->getMachineNode(X86::TEST32ri, dl,
> MVT::i32,
> - Subreg, Imm);
> + // Emit a testl or testw.
> + SDNode *NewNode = CurDAG->getMachineNode(Op, dl, MVT::i32, Reg,
> Imm);
> // Replace SUB|CMP with TEST, since SUB has two outputs while
> TEST has
> // one, do not call ReplaceAllUsesWith.
> ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
>
> Modified: llvm/trunk/lib/Target/X86/X86InstrArithmetic.td
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrArithmetic.td?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86InstrArithmetic.td (original)
> +++ llvm/trunk/lib/Target/X86/X86InstrArithmetic.td Mon Jan 29 12:54:33
> 2018
> @@ -1257,14 +1257,6 @@ let isCompare = 1 in {
> def TEST32mi : BinOpMI_F<0xF6, "test", Xi32, X86testpat, MRM0m>;
> let Predicates = [In64BitMode] in
> def TEST64mi32 : BinOpMI_F<0xF6, "test", Xi64, X86testpat, MRM0m>;
> -
> - // When testing the result of EXTRACT_SUBREG sub_8bit_hi, make sure
> the
> - // register class is constrained to GR8_NOREX. This pseudo is
> explicitly
> - // marked side-effect free, since it doesn't have an isel pattern like
> - // other test instructions.
> - let isPseudo = 1, hasSideEffects = 0 in
> - def TEST8ri_NOREX : I<0, Pseudo, (outs), (ins GR8_NOREX:$src,
> i8imm:$mask),
> - "", [], IIC_BIN_NONMEM>, Sched<[WriteALU]>;
> } // Defs = [EFLAGS]
>
> def TEST8i8 : BinOpAI_F<0xA8, "test", Xi8 , AL,
>
> Modified: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrInfo.cpp?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86InstrInfo.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86InstrInfo.cpp Mon Jan 29 12:54:33 2018
> @@ -8018,9 +8018,6 @@ bool X86InstrInfo::expandPostRAPseudo(Ma
> case X86::VMOVUPSZ256mr_NOVLX:
> return expandNOVLXStore(MIB, &getRegisterInfo(), get(X86::VMOVUPSYmr),
> get(X86::VEXTRACTF64x4Zmr), X86::sub_ymm);
> - case X86::TEST8ri_NOREX:
> - MI.setDesc(get(X86::TEST8ri));
> - return true;
> case X86::MOV32ri64:
> MI.setDesc(get(X86::MOV32ri));
> return true;
>
> Modified: llvm/trunk/lib/Target/X86/X86MacroFusion.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86MacroFusion.cpp?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86MacroFusion.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86MacroFusion.cpp Mon Jan 29 12:54:33 2018
> @@ -86,7 +86,6 @@ static bool shouldScheduleAdjacent(const
> case X86::TEST16mr:
> case X86::TEST32mr:
> case X86::TEST64mr:
> - case X86::TEST8ri_NOREX:
> case X86::AND16i16:
> case X86::AND16ri:
> case X86::AND16ri8:
>
> Modified: llvm/trunk/test/CodeGen/X86/test-shrink.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/test-shrink.ll?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/test-shrink.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/test-shrink.ll Mon Jan 29 12:54:33 2018
> @@ -484,8 +484,7 @@ no:
> define void @truncand32(i16 inreg %x) nounwind {
> ; CHECK-LINUX64-LABEL: truncand32:
> ; CHECK-LINUX64: # %bb.0:
> -; CHECK-LINUX64-NEXT: andl $2049, %edi # imm = 0x801
> -; CHECK-LINUX64-NEXT: testw %di, %di
> +; CHECK-LINUX64-NEXT: testl $2049, %edi # imm = 0x801
> ; CHECK-LINUX64-NEXT: je .LBB11_1
> ; CHECK-LINUX64-NEXT: # %bb.2: # %no
> ; CHECK-LINUX64-NEXT: retq
> @@ -498,8 +497,7 @@ define void @truncand32(i16 inreg %x) no
> ; CHECK-WIN32-64-LABEL: truncand32:
> ; CHECK-WIN32-64: # %bb.0:
> ; CHECK-WIN32-64-NEXT: subq $40, %rsp
> -; CHECK-WIN32-64-NEXT: andl $2049, %ecx # imm = 0x801
> -; CHECK-WIN32-64-NEXT: testw %cx, %cx
> +; CHECK-WIN32-64-NEXT: testl $2049, %ecx # imm = 0x801
> ; CHECK-WIN32-64-NEXT: je .LBB11_1
> ; CHECK-WIN32-64-NEXT: # %bb.2: # %no
> ; CHECK-WIN32-64-NEXT: addq $40, %rsp
> @@ -511,8 +509,7 @@ define void @truncand32(i16 inreg %x) no
> ;
> ; CHECK-X86-LABEL: truncand32:
> ; CHECK-X86: # %bb.0:
> -; CHECK-X86-NEXT: andl $2049, %eax # imm = 0x801
> -; CHECK-X86-NEXT: testw %ax, %ax
> +; CHECK-X86-NEXT: testl $2049, %eax # imm = 0x801
> ; CHECK-X86-NEXT: je .LBB11_1
> ; CHECK-X86-NEXT: # %bb.2: # %no
> ; CHECK-X86-NEXT: retl
>
> Modified: llvm/trunk/test/CodeGen/X86/testb-je-fusion.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/testb-je-fusion.ll?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/testb-je-fusion.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/testb-je-fusion.ll Mon Jan 29 12:54:33 2018
> @@ -6,9 +6,8 @@
> define i32 @check_flag(i32 %flags, ...) nounwind {
> ; CHECK-LABEL: check_flag:
> ; CHECK: # %bb.0: # %entry
> -; CHECK-NEXT: movl %edi, %ecx
> ; CHECK-NEXT: xorl %eax, %eax
> -; CHECK-NEXT: testb $2, %ch
> +; CHECK-NEXT: testl $512, %edi # imm = 0x200
> ; CHECK-NEXT: je .LBB0_2
> ; CHECK-NEXT: # %bb.1: # %if.then
> ; CHECK-NEXT: movl $1, %eax
>
> Modified: llvm/trunk/test/CodeGen/X86/vastart-defs-eflags.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vastart-defs-eflags.ll?rev=323690&r1=323689&r2=323690&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/vastart-defs-eflags.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/vastart-defs-eflags.ll Mon Jan 29 12:54:33
> 2018
> @@ -8,9 +8,7 @@ target triple = "x86_64-apple-macosx10.1
> define i32 @check_flag(i32 %flags, ...) nounwind {
> ; CHECK-LABEL: check_flag:
> ; CHECK: ## %bb.0: ## %entry
> -; CHECK-NEXT: pushq %rbx
> -; CHECK-NEXT: subq $48, %rsp
> -; CHECK-NEXT: movl %edi, %ebx
> +; CHECK-NEXT: subq $56, %rsp
> ; CHECK-NEXT: testb %al, %al
> ; CHECK-NEXT: je LBB0_2
> ; CHECK-NEXT: ## %bb.1: ## %entry
> @@ -29,7 +27,7 @@ define i32 @check_flag(i32 %flags, ...)
> ; CHECK-NEXT: movq %rdx, -{{[0-9]+}}(%rsp)
> ; CHECK-NEXT: movq %rsi, -{{[0-9]+}}(%rsp)
> ; CHECK-NEXT: xorl %eax, %eax
> -; CHECK-NEXT: testb $2, %bh
> +; CHECK-NEXT: testl $512, %edi ## imm = 0x200
> ; CHECK-NEXT: je LBB0_4
> ; CHECK-NEXT: ## %bb.3: ## %if.then
> ; CHECK-NEXT: leaq -{{[0-9]+}}(%rsp), %rax
> @@ -40,8 +38,7 @@ define i32 @check_flag(i32 %flags, ...)
> ; CHECK-NEXT: movl $8, 0
> ; CHECK-NEXT: movl $1, %eax
> ; CHECK-NEXT: LBB0_4: ## %if.end
> -; CHECK-NEXT: addq $48, %rsp
> -; CHECK-NEXT: popq %rbx
> +; CHECK-NEXT: addq $56, %rsp
> ; CHECK-NEXT: retq
> entry:
> %and = and i32 %flags, 512
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180130/ae94a3c0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: repro.ll
Type: application/octet-stream
Size: 3052 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180130/ae94a3c0/attachment.obj>
More information about the llvm-commits
mailing list