[PowerPC] Remove isTerminator for TRAP instruction

[qiucf]

Summary

In this patch https://github.ibm.com/compiler/llvm-project/commit/30e3db2 , the TRAP instruction for
PowerPC has been set to isTerminator. In fact, the TRAP instruction should not add isTerminator property,
because when the TRAP has been handled, the program should be continued.

This bug has caused 6 lit cases error on PPC.

For below case,

void test_builtin_trap() {
  volatile int i = 0;
  __builtin_trap();
  volatile int j = i;
}

we use clang sim.c -target powerpc-unknown-unknown -c to build it, we will get below error:

*** Bad machine code: Non-terminator instruction after the first terminator ***
- function:    test_builtin_trap
- basic block: %bb.0 entry (0x1002c61f1f8)
- instruction: %1:gprc = LWZ 0, %stack.0.i :: (volatile dereferenceable load 4 from %ir.i)
First terminator was:	TRAP

*** Bad machine code: Non-terminator instruction after the first terminator ***
- function:    test_builtin_trap
- basic block: %bb.0 entry (0x1002c61f1f8)
- instruction: STW killed %1:gprc, 0, %stack.1.j :: (volatile store 4 into %ir.j)
First terminator was:	TRAP
fatal error: error in backend: Found 2 machine code errors.

This patch is to fix above bug.

[ZhangKang]

Fix the case.

[efriedma]

Fixing a MachineVerifier issue by patching clang is generally wrong; if the IR is valid, the backend should do something sane with it. If the IR isn't valid, the IR Verifier should complain.

Here, I'd say the IR is valid; adding a special case to the IR rules here would be a pain. So the right fix involves changing isel and/or the definitions of the the instructions in question.

[ZhangKang]

Remoeve isTerminator for TRAP instruction.

[ZhangKang]

In D85599#2206033, @efriedma wrote:

Fixing a MachineVerifier issue by patching clang is generally wrong; if the IR is valid, the backend should do something sane with it. If the IR isn't valid, the IR Verifier should complain.

Here, I'd say the IR is valid; adding a special case to the IR rules here would be a pain. So the right fix involves changing isel and/or the definitions of the the instructions in question.

Yes, after the TRAP has been handled, the program should be continued. So I have removed the isTerminator for TRAP on PPC.

[shchenz]

With this change, we get verify failure for below case:

int test_builtin_trap(int num) {
  volatile int i = 0;
  if (num > 0)
    __builtin_unreachable();
  return i;
}

clang t.c -target powerpc-unknown-unknown  -c -mllvm -verify-machineinstrs -mllvm -trap-unreachable=true

*** Bad machine code: MBB exits via unconditional fall-through but ends with a barrier instruction! *** 
- function:    test_builtin_trap
- basic block: %bb.1 if.then (0x1001f837b50)
fatal error: error in backend: Found 1 machine code errors.

IR unreachable instruction is an IR level terminator, and it will be selected to TRAP in selectiondagbuilder, is it ok to change TRAP to non-terminator on PowerPC. If above case is valid, I think the answer is no.

There is no TRAP in IR level, I guess this is the reason why IR verifier does not complain. Setting llvm.trap as terminator seems also not right, because on some targets, it sets ISD::TRAP to non-terminator on purpose. See https://reviews.llvm.org/rL278144.

When selecting Intrinsic::trap to ISD::TRAP, updating the BB info according to TRAP is a terminator or not on specific target? If TRAP is terminator, add a new BB, if it is not, do nothing? I am not sure.

[nemanjai]

I think we should handle this similarly to SITargetLowering::splitKillBlock().

llvm/lib/Target/PowerPC/PPCInstrInfo.td

def STWCX : XForm_1_memOp<31, 150, (outs), (ins gprc:$rS, memrr:$dst),
                     "stwcx. $rS, $dst", IIC_LdStSTWCX, []>, isRecordForm;
 }
 
 let mayStore = 1, mayLoad = 0, hasSideEffects = 0 in
 def STWAT : X_RD5_RS5_IM5<31, 710, (outs), (ins gprc:$rS, gprc:$rA, u5imm:$FC),
                           "stwat $rS, $rA, $FC", IIC_LdStStore>,
             Requires<[IsISA3_0]>;
 
-let isTerminator = 1, isBarrier = 1, hasCtrlDep = 1 in
+let isBarrier = 1, hasCtrlDep = 1 in
 def TRAP  : XForm_24<31, 4, (outs), (ins), "trap", IIC_LdStLoad, [(trap)]>;
 
 def TWI : DForm_base<3, (outs), (ins u5imm:$to, gprc:$rA, s16imm:$imm),
                      "twi $to, $rA, $imm", IIC_IntTrapW, []>;
 def TW : XForm_1<31, 4, (outs), (ins u5imm:$to, gprc:$rA, gprc:$rB),
                  "tw $to, $rA, $rB", IIC_IntTrapW, []>;
 def TDI : DForm_base<2, (outs), (ins u5imm:$to, g8rc:$rA, s16imm:$imm),
                      "tdi $to, $rA, $imm", IIC_IntTrapD, []>;

llvm/test/CodeGen/PowerPC/builtins.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mcpu=pwr9 -mtriple=powerpc64le-unknown-unknown \
+; RUN:   -verify-machineinstrs < %s | FileCheck %s
+
+define void @test_builtin_trap() {
+; CHECK-LABEL: test_builtin_trap:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    li 3, 0
+; CHECK-NEXT:    stw 3, -4(1)
+; CHECK-NEXT:    trap
+; CHECK-NEXT:    lwz 3, -4(1)
+; CHECK-NEXT:    stw 3, -8(1)
+; CHECK-NEXT:    blr
+entry:
+  %i = alloca i32, align 4
+  %j = alloca i32, align 4
+  store volatile i32 0, i32* %i, align 4
+  call void @llvm.trap()
+  %0 = load volatile i32, i32* %i, align 4
+  store volatile i32 %0, i32* %j, align 4
+  ret void
+}
+
+declare void @llvm.trap()
+
+define void @test_builtin_debugtrap() {
+; CHECK-LABEL: test_builtin_debugtrap:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    li 3, 0
+; CHECK-NEXT:    stw 3, -4(1)
+; CHECK-NEXT:    trap
+; CHECK-NEXT:    lwz 3, -4(1)
+; CHECK-NEXT:    stw 3, -8(1)
+; CHECK-NEXT:    blr
+entry:
+  %i = alloca i32, align 4
+  %j = alloca i32, align 4
+  store volatile i32 0, i32* %i, align 4
+  call void @llvm.debugtrap()
+  %0 = load volatile i32, i32* %i, align 4
+  store volatile i32 %0, i32* %j, align 4
+  ret void
+}
+
+declare void @llvm.debugtrap()
+