Page MenuHomePhabricator

[SystemZ] Support LRVH and STRVH opcodes
ClosedPublic

Authored by zhanjunl on Apr 2 2016, 2:43 PM.

Details

Summary

On Linux, /usr/include/bits/byteswap-16.h defines __byteswap_16(x) as an inlined LRVH (Load Reversed Half-word) instruction. The SystemZ back-end did not support this opcode and the inlined assembly would cause a fatal error.

Diff Detail

Event Timeline

bryanpkc updated this revision to Diff 52480.Apr 2 2016, 2:43 PM
bryanpkc retitled this revision from to [SystemZ] Support LRVH and STRVH opcodes.
bryanpkc updated this object.
bryanpkc added a reviewer: uweigand.
bryanpkc added a subscriber: llvm-commits.
uweigand edited edge metadata.Apr 4 2016, 5:01 AM

Your change to README.txt implies that LRVH and STRVH still are not generated by the compiler. However, the change to SystemZInstrInfo.td actually installs DAG patterns for the SelectionDAG pass to recognize, so this is somewhat confusing.

Do those DAG patterns actually not match? If so, it might be better to not provide any. (Or of course even better, provide ones that actually work.) Or do they in fact match, and you just didn't verify that? If so, it would be good update README.txt accordingly and add a CodeGen test to verify correct code generation.

In any case, adding new instructions requires adding tests that they assemble and disassemble correctly, in test/MC/SystemZ/insn-*.s and test/MC/Disassembler/SystemZ/insns*.txt, respectively.

Your change to README.txt implies that LRVH and STRVH still are not generated by the compiler. However, the change to SystemZInstrInfo.td actually installs DAG patterns for the SelectionDAG pass to recognize, so this is somewhat confusing.

Oops, that wasn't intentional; I had only wanted to get the assembly parser working.

Do those DAG patterns actually not match? If so, it might be better to not provide any. (Or of course even better, provide ones that actually work.) Or do they in fact match, and you just didn't verify that? If so, it would be good update README.txt accordingly and add a CodeGen test to verify correct code generation.

I think it would be better to make the codegen emit LRVH and STRVH properly. I will verify that the patterns match correctly and add some test cases. Was there a reason why README.txt explicitly stated that LRVH and STRVH were not supported?

Thanks.

Do those DAG patterns actually not match? If so, it might be better to not provide any. (Or of course even better, provide ones that actually work.) Or do they in fact match, and you just didn't verify that? If so, it would be good update README.txt accordingly and add a CodeGen test to verify correct code generation.

I think it would be better to make the codegen emit LRVH and STRVH properly. I will verify that the patterns match correctly and add some test cases. Was there a reason why README.txt explicitly stated that LRVH and STRVH were not supported?

Well, it's a simple fact that in the current code they aren't supported. The more interesting question is, why didn't Richard just add them right away when he added the others? Unfortunately I don't recall the reason for that, it's been this way since the beginning.

One potential hick-up may be that i16 is not a legal type in the back-end, which means that the DAG may not actually contain a plain "load" node, but some form of an extending load. This would mean the matching DAG pattern would have to be updated accordingly.

zhanjunl commandeered this revision.May 12 2016, 10:57 AM
zhanjunl added a reviewer: bryanpkc.
zhanjunl edited edge metadata.

Updated Bryan's patch to use the DAG combiner to combine BSWAP+LOAD to SystemZISD::LRV and STORE+BSWAP to SystemZISD::STRV, and add patterns to match these into the correct LRVH/LRV/LRVG or STRVH/STRV/STRVG opcodes.

See a couple of inline comments. Otherwise this looks good ... Thanks!

lib/Target/SystemZ/SystemZISelLowering.cpp
4980

Why is this check needed?

lib/Target/SystemZ/SystemZISelLowering.h
318

Please document the operands of those new ISD codes here.

lib/Target/SystemZ/SystemZOperators.td
205

Don't these need the SDNPMemOperand flag as well?

Updated the patch to add context.

Made changes to the diff based on Ulrich's comments:

Removed the unindexed check because it was unnecessary.
Added comments to describe the operands of the new LRV and STRV ISDs.
Added a comment to explain the need for the !volatile check.
Added the SDNPMemOperand property to the node definitions.

zhanjunl marked 3 inline comments as done.May 13 2016, 7:36 AM
uweigand accepted this revision.May 13 2016, 10:45 AM
uweigand edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.May 13 2016, 10:45 AM
bryanpkc closed this revision.May 16 2016, 1:38 PM