Page MenuHomePhabricator

[SystemZ] Add support for missing instructions
ClosedPublic

Authored by zhanjunl on Jul 6 2016, 9:19 AM.

Details

Summary

Add support to allow clang integrated assembler to recognize some missing instructions, for openssl.

Instructions are:
LM, LMH, LMY, STM, STMH, STMY, ICM, ICMH, ICMY, SLA, SLAK, TML, TMH, EX, EXRL.

Event Timeline

zhanjunl retitled this revision from to [SystemZ] Add support for missing instructions.
zhanjunl updated this object.
zhanjunl added a reviewer: uweigand.
zhanjunl added a subscriber: llvm-commits.
uweigand edited edge metadata.Jul 6 2016, 11:09 AM

See inline comments.

Also, when adding new instructions, you need to add disassembler tests as well (test/MC/Disassembler/SystemZ).

It would be nice to add not just single instructions you find missing somewhere, but full related groups of instructions where it makes sense. For example, when adding EX, it might be nice to also add EXRL. Similarly, when adding ICM(Y), it would be nice to also add ICMH, and possibly even STCM(Y/H) and CLCM(Y/H) as well. (To be clear: this is just a suggestion, not a requirement.)

lib/Target/SystemZ/SystemZInstrInfo.td
706 ↗(On Diff #63240)

This should use a *Pair pattern like all other "y" instructions.

711 ↗(On Diff #63240)

Likewise.

825 ↗(On Diff #63240)

Please create a new TernaryRSPair for those (implemented similarly to TernaryRXF).

1379 ↗(On Diff #63240)

These are just aliases for TMLL/TMLH, so it should be enough to define those via InstAlias.

1667 ↗(On Diff #63240)

Don't need the { } for a single statement here.

koriakin added inline comments.
lib/Target/SystemZ/SystemZInstrInfo.td
1199 ↗(On Diff #63240)

Don't give a pattern to this instruction - otherwise, ISel could use it to implement shl, which is incorrect.

zhanjunl edited edge metadata.

Updated the diff with fixes based on comments, to add *Pair classes, to use InstAlias, and to remove the pattern from SLA. Also added EXRL, ICMH, STMH, LMH, and added disassembler tests.

uweigand accepted this revision.Jul 8 2016, 8:46 AM
uweigand edited edge metadata.

See the inline comment. Otherwise, LGTM. Thanks!

lib/Target/SystemZ/SystemZInstrInfo.td
823 ↗(On Diff #63240)

ICMH also should be in the Defs = [CC] clause.

This revision is now accepted and ready to land.Jul 8 2016, 8:46 AM
zhanjunl updated this object.
zhanjunl edited edge metadata.

Whoops you're right, I've added ICMH to the Defs = [CC] clause.

zhanjunl marked 7 inline comments as done.Jul 8 2016, 9:06 AM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.