This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] refactor to remove remove AsmVariant. NFC
ClosedPublic

Authored by nickdesaulniers on Apr 9 2019, 2:46 PM.

Details

Summary

The InlineAsm::AsmDialect is only required for X86; no architecture
makes use of it and as such it gets passed around between arch-specific
and general code while being unused for all architectures but X86.

Since the AsmDialect is queried from a MachineInstr, which we also pass
around, remove the additional AsmDialect parameter and query for it deep
in the X86AsmPrinter only when needed/as late as possible.

This refactor should help later planned refactors to AsmPrinter, as this
difference in the X86AsmPrinter makes it harder to make AsmPrinter more
generic.

Event Timeline

nickdesaulniers created this revision.Apr 9 2019, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 2:46 PM
  • git-clang-format HEAD~
echristo accepted this revision.Apr 9 2019, 3:13 PM

One inline comment for a future cleanup, otherwise LGTM and thanks!

-eric

llvm/lib/Target/X86/X86AsmPrinter.cpp
230

... you can get it on the MI? That seems like the wrong level :)

This revision is now accepted and ready to land.Apr 9 2019, 3:13 PM
craig.topper added inline comments.Apr 9 2019, 3:15 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
444

It doesn't make sense to me that we were passing InlineAsmVariant(which should always be AT&T) here. Shouldn't we be passing the PrinterAsmVariant? If the printer is set to Intel syntax and we expand dialects from curly braces assuming that. Then we should also print operands using Intel syntax. And then tell the MC layer that what it needs to parse is Intel syntax.

ychen added a subscriber: ychen.Apr 9 2019, 3:28 PM
ychen added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
444

I think this is related to PR23933 (which I'm actively working on) and PR24232. I will submit a review for PR23933 very soon.

nickdesaulniers marked an inline comment as done.
  • add FIXME: as per echristo
This revision was automatically updated to reflect the committed changes.

Note to self: merge the arc clone branch, or squash the commits in the local branch before committing.