This is an archive of the discontinued LLVM Phabricator instance.

[AArch64InstPrinter] Introduce register markup tags emission
ClosedPublic

Authored by antoniofrighetto on Jul 15 2022, 8:54 AM.

Details

Summary

AArch64 assembly syntax emission now leverages markup tags for registers, if enabled.

Diff Detail

Event Timeline

antoniofrighetto requested review of this revision.Jul 15 2022, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 8:54 AM
david-arm accepted this revision.Aug 11 2022, 1:44 AM

LGTM!

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1473–1474

nit: You could write this in fewer lines I think with just:

if (MRI.getRegClass(AArch64::ZPRRegClassID).contains(Reg))
  printRegName(O, Reg);
else
  printRegName(O, Reg, AArch64::vreg);
O << LayoutSuffix;
This revision is now accepted and ready to land.Aug 11 2022, 1:44 AM
MaskRay accepted this revision.Aug 11 2022, 12:07 PM
antoniofrighetto marked an inline comment as done.

@david-arm, thanks for the nit, had missed this. I just updated the diff w/ that change included. Mind committing it on my behalf (GitHub account associated to this profile, as I do not have commit access)? Thank you.

Hmmm sorry, I hadn't seen this when pushing 1b726f0a4ceec0747782545f3d4727f5914ee04b . This needs a rebase and ideally this patch can add a few tests.

@MaskRay, yeah, sorry, I should have probably specified it :/ Essentially, the other commit 4e9907977465 was relying on this one, so this one should have been pushed before, as the other contained the test (which leverages on this change too). Hence, there should be no need to actually introduce tests for this one, as already introduced in the other commit.

You may need to rebase this patch now. I think this changes many instructions not covered by MC/Disassembler/AArch64/marked-up.txt. Having better coverage will be appreicated.

@MaskRay, rebased to trunk & added more coverage test!

llvm/test/MC/Disassembler/AArch64/marked-up.txt fails.
See Build 280780: pre-merge checks x64 windows failed · x64 debian failed

The last update still failed the test. If the patch has a dependency, you can click "Edit Related Revisions" to state so.

llvm/test/MC/Disassembler/AArch64/marked-up.txt
16

It's better to use an instruction family name

For st64b, I pick the extension name.

@MaskRay, fixed by extending properly the patch. Coverage test preserved, and added a further one for completeness.

This revision was landed with ongoing or failed builds.Sep 13 2022, 8:52 PM
This revision was automatically updated to reflect the committed changes.