This is an archive of the discontinued LLVM Phabricator instance.

[SystemZInstPrinter] Introduce markup tags emission
ClosedPublic

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

Details

Summary

SystemZ assembly syntax emission now leverages markup tags, if enabled.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 8:47 AM
antoniofrighetto requested review of this revision.Jul 15 2022, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 8:47 AM

Can you explain what this is, and where this marked-up assembler could be useful? I see so far only ARM supports that feature, but I don't really understand the point ...

antoniofrighetto added a comment.EditedAug 26 2022, 7:01 AM

@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.

Fixed a small regression.

@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.

Do you have any example of such tools? The reason I'm asking is that it's a bit hard for me to judge whether the implementation is fully complete and would make those uses cases actually work, without seeing any.

As a comment on the implementation: I think it would be much preferable to use the base class UseMarkup member (or even better, the markup helper), instead of having to explicitly pass on the extra argument. I guess the reason you're not doing this is because of the calls from PrintAsmOperand / PrintMemAsmOperand? This actually makes me wonder if it wouldn't be cleaner to just duplicate the (small amount of) code in printOperand and printAddress into PrintAsmOperand and PrintMemAsmOperand, and then stop making all those SystemZInstrPrinter routines static ...

@uweigand, we leverage LLVM disassembler (and, specifically, MCInstPrinter::printInst) as part of our static binary translator tool, here, though I assume I could count many other uses that I'm not aware of (we've updated other targets too in order to support markup).

I'm actually using UseMarkup member field anywhere, except for the static functions – whose callers are PrintAsmOperand / PrintMemAsmOperand, as you noted – for which, clearly, I could not. I reckon it's not really that big of a deal, being limited to a very small portion of the code.

Test fixed.

@MaskRay, test added for this patch too, failure of unit tests seems completely unrelated to the change.

antoniofrighetto added a comment.EditedSep 19 2022, 1:16 PM

@MaskRay, as there do not seem to be further concerns, could you kindly commit this on my behalf please (I do not have commit access)?

MaskRay accepted this revision.Sep 19 2022, 2:41 PM
This revision is now accepted and ready to land.Sep 19 2022, 2:41 PM

@MaskRay, as there do not seem to be further concerns, could you kindly commit this on my behalf please (I do not have commit access)?

I've LGTM but we need to wait a bit for @uweigand

Sorry for the delay, I was traveling last week. Thanks for the use case, it's good to have some validation that the output in this form is actually usable.

As to the implementation, I still believe it would be better to eliminate the static routines as outlined in my last comment. Do you think you could give that a try?

Hello @uweigand, I apologize for being late on this. From the build, it looks like some systemz tests are failing because the register name is actually not emitted. Is it possible that I'm not handling properly it via SystemZInstPrinter::getRegisterName? Do you have any clue by any chance? The duplicated code in SystemZAsmPrinter.cpp looks correct to me.

uweigand added inline comments.Oct 24 2022, 6:20 AM
llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp
70

You've removed the RegName here.

81–82

Could you make those non-static as well, so we don't have to pass in UseMarkup explicitly.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
796

And RegName is also missing here.

antoniofrighetto marked 3 inline comments as done.
antoniofrighetto added inline comments.
llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp
70

Argh, how could I not even notice? Fixed, thanks for noticing it.

81–82

Sure!

uweigand accepted this revision.Oct 25 2022, 9:51 AM

LGTM now! Thanks for making those changes.

antoniofrighetto marked 2 inline comments as done.EditedOct 25 2022, 9:53 AM

@uweigand, thanks for reviewing them! Might I ask you to commit it on my behalf, please (as I do not have commit access, account associated to GitHub)? Thank you.

This revision was automatically updated to reflect the committed changes.