This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Support Compare and Traps
ClosedPublic

Authored by zhanjunl on Jun 8 2016, 2:08 PM.

Details

Summary

Support and generate Compare and Traps like CRT, CIT, etc.

Support Trap as legal DAG opcodes and generate "j .+2" for them by default.
Add support for Conditional Traps and use the If Converter to convert them into the
corresponding compare and trap opcodes.

Diff Detail

Event Timeline

zhanjunl retitled this revision from to [SystemZ] Support Compare and Traps.
zhanjunl updated this object.
zhanjunl added a subscriber: llvm-commits.
koriakin edited edge metadata.Jun 8 2016, 2:24 PM

In addition to inline comments: there is no test for unconditional trap, or for non-fused conditional trap (you can get that one easily by a floating point comparison).

lib/Target/SystemZ/SystemZInstrFormats.td
223

nit: would be good to have them in order: InstRIEa, InstRIEb, InstRIEc.

lib/Target/SystemZ/SystemZInstrInfo.td
71

Alias<4 would be slightly more correct, though I don't think anything actually uses size information for aliases

74

This is not a barrier, and it does use custom inserter.

170

Not barriers.

293

Again, not barriers.

Oh, and remove the TODO item from lib/Target/SystemZ/README while you're at it :)

lib/Target/SystemZ/SystemZAsmPrinter.cpp
435

Nit: "jump to that label" would mean an infinite loop, say "use that label" or "jump to that label + 2" instead.

Oh, and there should be MC tests for the fused compares (an assembler test and a disassembler test).

It's also worth noting that there are also two more fused ops that could be supported here: CLT and CLGT (they don't have compare-and-branch variants), though they are a bit of a mess (they have different memory operands wrt their compare equivalents), so might be out of scope for this patch.

test/CodeGen/SystemZ/trap-01.ll
26

I'm not sure G stands for "grande", but I like it either way :) Got a good explanation for F?

uweigand edited edge metadata.Jun 9 2016, 7:21 AM

Thanks to koriakin for the review, I agree with the points raised there.

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?

Also, LLVM coding style limits line length to 80 characters, which your patch currently exceed in some places.

zhanjunl edited edge metadata.

Thanks Marcin and Ulrich for the quick reviews. I've addressed most of them, and fixed the line width to 80 characters.

Ordered the InstRIEa instruction class properly now.
Set the alias size correctly.
Removed isBarrier from CondTrap and the Compare and Trap instruction sets.
Fixed the comment to "use that label instead".
Removed the TODO from the README file.
Added MC assembler and disassembler tests, and added tests for unconditional traps and invalid Compare and Trap, to test CondTrap.

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.

zhanjunl marked 6 inline comments as done.Jun 9 2016, 11:35 AM
lib/Target/SystemZ/SystemZInstrInfo.td
74

I'm not sure why this should be marked as usesCustomInserter? The Trap instruction needed it because I needed to split the block, but CondTrap doesn't really need it since it should only have been inserted at the end of a basic block.

test/CodeGen/SystemZ/trap-01.ll
26

I have no explanation for F :(

koriakin added inline comments.Jun 9 2016, 11:46 AM
lib/Target/SystemZ/SystemZInstrInfo.td
74

Whoops, that's right, my bad.

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?

I've unmarked isTerminator from the Trap instruction for now, and removed all custom insertion code. I'll bring it up in the mailing list and see if I can't find out where it should be fixed in the common code.

uweigand accepted this revision.Jun 10 2016, 11:45 AM
uweigand edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jun 10 2016, 11:45 AM
This revision was automatically updated to reflect the committed changes.