This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][TableGen] Skip tied result operands for InstAlias
ClosedPublic

Authored by sdesmalen on Nov 14 2017, 8:23 AM.

Details

Summary

This patch fixes an issue so that the right alias is printed when the instruction has tied operands. It checks the number of operands in the resulting instruction as opposed to the alias, and then skips over tied operands that should not be printed in the alias.

This allows to generate the preferred assembly syntax for the AArch64 'ins' instruction, which should always be displayed as 'mov' according to the ARM Architecture Reference Manual. Several unit tests have changed as a result, but only to reflect the preferred disassembly. Some other InstAlias patterns (movk/bic/orr) needed a slight adjustment to stop them becoming the default and breaking other unit tests.

Please note that the patch is mostly the same as https://reviews.llvm.org/D29219 which was reverted because of an issue found when running TableGen with the Address Sanitizer. That issue has been addressed in this iteration of the patch.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 14 2017, 8:23 AM
SjoerdMeijer accepted this revision.Nov 15 2017, 12:26 AM

Looks like a straightforward fix of D29219 to me. But perhaps wait a little bit with committing, just in case @rengolin who did the original review has some more comments.

utils/TableGen/AsmWriterEmitter.cpp
823

Nit: do we need to set/use 'e' here?

This revision is now accepted and ready to land.Nov 15 2017, 12:26 AM
fhahn added a subscriber: fhahn.Nov 16 2017, 6:53 AM

just 2 nits that might be worth considering. Otherwise LGTM as a fix for the reverted D29219

utils/TableGen/AsmWriterEmitter.cpp
823

How about

for (auto &OpInfo : CGA.ResultInst->Operands)
  NumMIOps += OpInfo.MINumOperands;

:)

836

nit: maybe it would be worth adding a variable for CGA.ResultInst->Operands and using that instead the longer explicit access

rengolin accepted this revision.Nov 18 2017, 12:29 PM

LGTM, too.

sdesmalen updated this revision to Diff 123575.Nov 20 2017, 6:16 AM

Addressed two nits to make code more readable.

sdesmalen closed this revision.Nov 20 2017, 6:36 AM