[llvm] r306106 - [SystemZ] Fix trap issue and enable expensive checks.

Fri Jun 23 10:57:11 PDT 2017

Hi Krzysztof,

When I looked at the buildbots page, it looked like it was broken before 
my commit.

If it was my commit that caused the problem on your side, that's 
strange, because I ran it with and without EXPENSIVE_CHECKS on SystemZ 
for all targets, which passed. I then also ran those tests on my laptop 
for X86, which worked fine.

Since this is a generic test, there should not be any difference for any 
target. What I did was simply to add the machine-verifier to that test 
so that it will work properly regardless of EXPENSIVE_CHECKS. If 
building with EXPENSIVE_CHECKS, the machine verifier will be inserted in 
the pass list, and tests that check for the pass list output, or pass 
arguments list, did not work before...

Please let me know if I can help in any way. CC:ing Simon who might be 
able to help,

Jonas

On 2017-06-23 19:49, Krzysztof Parzyszek wrote:
> Hi Jonas,
> This broke Hexagon bots:
>
> http://lab.llvm.org:8011/builders/llvm-hexagon-elf/builds/7304
>
> -Krzysztof
>
>
> On 6/23/2017 9:30 AM, Jonas Paulsson via llvm-commits wrote:
>> Author: jonpa
>> Date: Fri Jun 23 09:30:46 2017
>> New Revision: 306106
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=306106&view=rev
>> Log:
>> [SystemZ]  Fix trap issue and enable expensive checks.
>>
>> The isBarrier/isTerminator flags have been removed from the SystemZ trap
>> instructions, so that tests do not fail with EXPENSIVE_CHECKS. This 
>> was just
>> an issue at -O0 and did not affect code output on benchmarks.
>>
>> (Like Eli pointed out: "targets are split over whether they consider 
>> their
>> "trap" a terminator; x86, AArch64, and NVPTX don't, but ARM, MIPS, 
>> PPC, and
>> SystemZ do. We should probably try to be consistent here.". This is 
>> still the
>> case, although SystemZ has switched sides).
>>
>> SystemZ now returns true in isMachineVerifierClean() :-)
>>
>> These Generic tests have been modified so that they can be run with 
>> or without
>> EXPENSIVE_CHECKS: CodeGen/Generic/llc-start-stop.ll and
>> CodeGen/Generic/print-machineinstrs.ll
>>
>> Review: Ulrich Weigand, Simon Pilgrim, Eli Friedman
>> https://bugs.llvm.org/show_bug.cgi?id=33047
>> https://reviews.llvm.org/D34143
>>
>> Modified:
>>      llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.td
>>      llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h
>>      llvm/trunk/test/CodeGen/Generic/llc-start-stop.ll
>>      llvm/trunk/test/CodeGen/Generic/print-machineinstrs.ll
>>      llvm/trunk/test/CodeGen/SystemZ/trap-02.ll
>>
>> Modified: llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.td
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.td?rev=306106&r1=306105&r2=306106&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.td (original)
>> +++ llvm/trunk/lib/Target/SystemZ/SystemZInstrInfo.td Fri Jun 23 
>> 09:30:46 2017
>> @@ -189,18 +189,15 @@ let isBranch = 1, isTerminator = 1 in {
>> //===----------------------------------------------------------------------===//
>>     // Unconditional trap.
>> -// FIXME: This trap instruction should be marked as isTerminator, 
>> but there is
>> -// currently a general bug that allows non-terminators to be placed 
>> between
>> -// terminators. Temporarily leave this unmarked until the bug is fixed.
>> -let isBarrier = 1, hasCtrlDep = 1 in
>> +let hasCtrlDep = 1 in
>>     def Trap : Alias<4, (outs), (ins), [(trap)]>;
>>     // Conditional trap.
>> -let isTerminator = 1, hasCtrlDep = 1, Uses = [CC] in
>> +let hasCtrlDep = 1, Uses = [CC] in
>>     def CondTrap : Alias<4, (outs), (ins cond4:$valid, cond4:$R1), []>;
>>     // Fused compare-and-trap instructions.
>> -let isTerminator = 1, hasCtrlDep = 1 in {
>> +let hasCtrlDep = 1 in {
>>     // These patterns work the same way as for compare-and-branch.
>>     defm CRT   : CmpBranchRRFcPair<"crt",   0xB972, GR32>;
>>     defm CGRT  : CmpBranchRRFcPair<"cgrt",  0xB960, GR64>;
>>
>> Modified: llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h?rev=306106&r1=306105&r2=306106&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h (original)
>> +++ llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h Fri Jun 23 
>> 09:30:46 2017
>> @@ -51,8 +51,6 @@ public:
>>     }
>>       bool targetSchedulesPostRAScheduling() const override { return 
>> true; };
>> -
>> -  bool isMachineVerifierClean() const override { return false; }
>>   };
>>     } // end namespace llvm
>>
>> Modified: llvm/trunk/test/CodeGen/Generic/llc-start-stop.ll
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Generic/llc-start-stop.ll?rev=306106&r1=306105&r2=306106&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/test/CodeGen/Generic/llc-start-stop.ll (original)
>> +++ llvm/trunk/test/CodeGen/Generic/llc-start-stop.ll Fri Jun 23 
>> 09:30:46 2017
>> @@ -1,7 +1,10 @@
>> -; RUN: llc < %s -debug-pass=Structure -stop-after=loop-reduce -o 
>> /dev/null 2>&1 | FileCheck %s -check-prefix=STOP-AFTER
>> +; Note: -verify-machineinstrs is used in order to make this test 
>> compatible with EXPENSIVE_CHECKS.
>> +; RUN: llc < %s -debug-pass=Structure -stop-after=loop-reduce 
>> -verify-machineinstrs -o /dev/null 2>&1 \
>> +; RUN:   | FileCheck %s -check-prefix=STOP-AFTER
>>   ; STOP-AFTER: -loop-reduce
>>   ; STOP-AFTER: Dominator Tree Construction
>>   ; STOP-AFTER: Loop Strength Reduction
>> +; STOP-AFTER-NEXT: Verify generated machine code
>>   ; STOP-AFTER-NEXT: MIR Printing Pass
>>     ; RUN: llc < %s -debug-pass=Structure -stop-before=loop-reduce -o 
>> /dev/null 2>&1 | FileCheck %s -check-prefix=STOP-BEFORE
>>
>> Modified: llvm/trunk/test/CodeGen/Generic/print-machineinstrs.ll
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Generic/print-machineinstrs.ll?rev=306106&r1=306105&r2=306106&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/test/CodeGen/Generic/print-machineinstrs.ll (original)
>> +++ llvm/trunk/test/CodeGen/Generic/print-machineinstrs.ll Fri Jun 23 
>> 09:30:46 2017
>> @@ -1,12 +1,25 @@
>> -; RUN: llc < %s -O3 -debug-pass=Structure 
>> -print-machineinstrs=branch-folder -o /dev/null 2>&1 | FileCheck %s
>> -; RUN: llc < %s -O3 -debug-pass=Structure -print-machineinstrs -o 
>> /dev/null 2>&1 | FileCheck %s
>> -; RUN: llc < %s -O3 -debug-pass=Structure -print-machineinstrs= -o 
>> /dev/null 2>&1 | FileCheck %s
>> +; RUN: llc < %s -O3 -debug-pass=Structure 
>> -print-machineinstrs=branch-folder -verify-machineinstrs -o /dev/null 
>> 2>&1 \
>> +; RUN:   | FileCheck %s -check-prefix=PRINT-BRANCH-FOLD
>> +; RUN: llc < %s -O3 -debug-pass=Structure -print-machineinstrs 
>> -verify-machineinstrs -o /dev/null 2>&1 \
>> +; RUN:   | FileCheck %s -check-prefix=PRINT
>> +; RUN: llc < %s -O3 -debug-pass=Structure -print-machineinstrs= 
>> -verify-machineinstrs -o /dev/null 2>&1 \
>> +; RUN:   | FileCheck %s -check-prefix=PRINT
>> +
>> +; Note: -verify-machineinstrs is used in order to make this test 
>> compatible with EXPENSIVE_CHECKS.
>>     define i64 @foo(i64 %a, i64 %b) nounwind {
>> -; CHECK: -branch-folder  -machineinstr-printer
>> -; CHECK: Control Flow Optimizer
>> -; CHECK-NEXT: MachineFunction Printer
>> -; CHECK: Machine code for function foo:
>> +; PRINT-BRANCH-FOLD: -branch-folder -machineverifier 
>> -machineinstr-printer
>> +; PRINT-BRANCH-FOLD: Control Flow Optimizer
>> +; PRINT-BRANCH-FOLD-NEXT: Verify generated machine code
>> +; PRINT-BRANCH-FOLD-NEXT: MachineFunction Printer
>> +; PRINT-BRANCH-FOLD: Machine code for function foo:
>> +
>> +; PRINT: -branch-folder -machineinstr-printer
>> +; PRINT: Control Flow Optimizer
>> +; PRINT-NEXT: MachineFunction Printer
>> +; PRINT-NEXT: Verify generated machine code
>> +; PRINT: Machine code for function foo:
>> +
>>     %c = add i64 %a, %b
>>     %d = trunc i64 %c to i32
>>     %e = zext i32 %d to i64
>>
>> Modified: llvm/trunk/test/CodeGen/SystemZ/trap-02.ll
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/SystemZ/trap-02.ll?rev=306106&r1=306105&r2=306106&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/test/CodeGen/SystemZ/trap-02.ll (original)
>> +++ llvm/trunk/test/CodeGen/SystemZ/trap-02.ll Fri Jun 23 09:30:46 2017
>> @@ -47,9 +47,9 @@ if.end:
>>   define i32 @f3(i32 zeroext %a, i32 *%base, i64 %offset) {
>>   ; CHECK-LABEL: f3:
>>   ; CHECK: cl %r2, 0(%r{{[0-5]}},%r3)
>> +; CHECK: lhi %r2, 0
>>   ; CHECK-LABEL: .Ltmp0
>>   ; CHECK: jh .Ltmp0+2
>> -; CHECK: lhi %r2, 0
>>   ; CHECK: br %r14
>>   entry:
>>     %ptr = getelementptr i32, i32 *%base, i64 %offset
>> @@ -70,9 +70,9 @@ if.end:
>>   define i64 @f4(i64 %a, i64 *%base, i64 %offset) {
>>   ; CHECK-LABEL: f4:
>>   ; CHECK: clg %r2, 0(%r{{[0-5]}},%r3)
>> +; CHECK: lghi %r2, 0
>>   ; CHECK-LABEL: .Ltmp1
>>   ; CHECK: jh .Ltmp1+2
>> -; CHECK: lghi %r2, 0
>>   ; CHECK: br %r14
>>   entry:
>>     %ptr = getelementptr i64, i64 *%base, i64 %offset
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>