[PATCH] D21155: [SystemZ] Support Compare and Traps

[Ulrich Weigand via llvm-commits]

uweigand added a comment.

In http://reviews.llvm.org/D21155#453635, @zhanjunl wrote:

> In http://reviews.llvm.org/D21155#453347, @uweigand wrote:
>
> > One additional question: why is the splitBlockAfter in emitTrap needed?   I notice that other platforms don't seem to require similar code when emitting a trap.  Shouldn't marking the trap as isTerminator in the .td file cause common code to terminate the basic block already?
>
>
> This part of the patch I was unsure of, because I'm not sure if I'm using the custom inserter properly. When compiling this LLVM IR:
>
>   define signext i32 @f0() {
>   entry:
>     call void @llvm.trap()
>     ret i32 0
>   }
>
>
> Other platforms like PowerPC can compile that code fine, essentially generating "TRAP, LOAD 0, RETURN". SystemZ will throw an assertion, because there is a non-terminator between terminators (LOAD 0). I think the assertion makes sense, but I couldn't find any common code that terminated a basic block after seeing a terminator, which is why I had to use the custom inserter.


Hmm, it seems this in indeed incorrect on all platforms, it's just that the bug is silent by default except on SystemZ, where we run into an assertion in /SystemZLongBranch.cpp.  However, you can see the bug on all platforms (I've tested powerpc64le and amd64) by using llc -verify-machineinstrs.  This will report:

- Bad machine code: Non-terminator instruction after the first terminator ***

after the Instruction Selection pass.

Note that with -verify-machineinstrs, even in the presence of the emitTrap custom inserter on SystemZ, llc -verify-machineinstrs will *still* report the error, since the invalid machine code is still present initially after the instruction selection pass (even though it is cleaned up later by the custom inserter).

I think the correct place to fix this would be in common code, either during isel, or even at the IR stage.  (Why is there any code after a call to a noreturn builtin in the first place?)   It might be best to start a discussion on this on the mailing list.

In the meantime, maybe the preferable workaround would be to not mark the trap instruction as isTerminator?


http://reviews.llvm.org/D21155