Page MenuHomePhabricator

Use printAliasInstr in ARM target
ClosedPublic

Authored by rogfer01 on May 13 2016, 7:04 AM.

Details

Summary

ARM target does not use printAliasInstr machinery which
forces having special checks in ArmInstPrinter::printInstruction. This
patch addresses this issue.

Not all special checks could be removed: either they involve elaborated
conditions under which the alias is emitted (e.g. ldm/stm on sp may be
pop/push but only if the number of registers is >= 2) or the number
of registers is multivalued (like happens again with ldm/stm) and they
do not match the InstAlias pattern which assumes single-valued operands
in the pattern.

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 updated this revision to Diff 57164.May 13 2016, 7:04 AM
rogfer01 retitled this revision from to Use printAliasInstr in ARM target.
rogfer01 updated this object.
rogfer01 added a reviewer: t.p.northover.
rogfer01 added a subscriber: llvm-commits.

Ping?

Thanks!

Hi Roger,

Sorry, that completely slipped through the net. Thanks for the ping.

This change looks good, but the flag name "Emit" seems dubious. It creates the idea, just by looking at the table-gen, that all the other aliases don't need to be emitted. Now that it's being used more widely, a more expressive name would help understand its real purpose?

cheers,
--renato

Hi Renato,

thanks for the review. I'll try to come up with a better name for Emit.

Regards,
Roger

Well, after checking the definition in Target.td it seems that Emit is the name of the parameter for the InstAlias::EmitPriority attribute. If it is zero, the alias is never printed, and if it is nonzero then the higher priority alias will be printed.

In the ARM target we are using it like a boolean (0 or 1, no priorities involved). So maybe we may want to use a name like PrintAlias. But this name will become awkward if the priority feature ends being used.

Suggestions?

Thanks,

Maybe just EmitPriority or Priority would do. Even if we're not using it now, that's what it means.

Probably a comment on the class definition that the priority forces the automatic emission to print the aliases.

cheers,
--renato

rogfer01 updated this revision to Diff 57941.EditedMay 20 2016, 9:28 AM

Rename Emit to EmitPriority for clarity

Ping?

Kind regards,

Ping?

Thanks!

rengolin accepted this revision.May 31 2016, 3:54 AM
rengolin added a reviewer: rengolin.

Sorry, holidays... LGTM, thanks!

Don't forget to add "NFC" on the commit title.

This revision is now accepted and ready to land.May 31 2016, 3:54 AM
This revision was automatically updated to reflect the committed changes.