Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llc] Add -M for InstPrinter options #121078

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Dec 25, 2024

For many targets, llvm-objdump and llvm-mc
(https://reviews.llvm.org/D103004) support -M no-aliases (e.g.
RISCVInstPrinter::applyTargetSpecificCLOption).

This patch implements -M for llc.

While here, rename "DisassemblerOptions" in llvm-mc to the more
appropriate "InstPrinterOptions". For llvm-mc --assemble, there is no
disassembler involved.

Created using spr 1.3.5-bogner
Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it is currently implemented it only works when invoking llc directly.
It would be great if the option could be set by the compiler driver, which passes options in memory.
See

MCOptions.AsmVerbose = true;
for a few examples.

Some targets prefer to not print aliases by default; there should be a way of enabling this behavior.

Comment on lines +171 to +172
return createStringError(inconvertibleErrorCode(),
"invalid InstPrinter option '" + Opt + "'");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


(nit) There is an overload accepting Twine:

Suggested change
return createStringError(inconvertibleErrorCode(),
"invalid InstPrinter option '" + Opt + "'");
return createStringError("invalid InstPrinter option '" + Opt + "'");

Comment on lines +136 to +137
if (auto Err = MCStreamerOrErr.takeError()) {
Context.reportError(SMLoc(), toString(std::move(Err)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


(nit) Could avoid moving the Error twice:

Suggested change
if (auto Err = MCStreamerOrErr.takeError()) {
Context.reportError(SMLoc(), toString(std::move(Err)));
if (!MCStreamerOrErr) {
Context.reportError(SMLoc(), toString(MCStreamerOrErr.takeError()));

@MaskRay
Copy link
Member Author

MaskRay commented Dec 25, 2024

The way it is currently implemented it only works when invoking llc directly. It would be great if the option could be set by the compiler driver, which passes options in memory. See

MCOptions.AsmVerbose = true;

for a few examples.
Some targets prefer to not print aliases by default; there should be a way of enabling this behavior.

Thanks for the quick response. This patch intends to port applyTargetSpecificCLOption to llc to be on par with llvm-mc and llvm-objdump.

You are right that Clang cc1as sets MCTargetOptions in a different place. If we want to add a clang -cc1 and -cc1as option, the clang file needs to be modified. That isn't my motivation, though. (Skimming through GCC's manpage I don't find a similar option. If we add an option, we have some freedom on the naming.)

@s-barannikov
Copy link
Contributor

I'm not against the current approach, it is just I'm not sure that this option would be as useful for llc without allowing it to be forwarded from the driver. llc is more a developer tool than an end-user tool. I doubt developers will want to pass this option to llc (what for?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants