This is an archive of the discontinued LLVM Phabricator instance.

Demote EmitRawText call in AsmPrinter::EmitInlineAsm() and remove hasRawTextSupport() call
ClosedPublic

Authored by dsanders on Feb 4 2014, 5:48 AM.

Details

Summary

AsmPrinter::EmitInlineAsm() will no longer use the EmitRawText() call for targets with mature MC support. Such targets will always parse the inline assembly (even when emitting assembly). Targets without mature MC support continue to use EmitRawText() for assembly output.

The hasRawTextSupport() check in AsmPrinter::EmitInlineAsm() has been replaced with MCAsmInfo::UseIntegratedAs which when true, causes the integrated assembler to parse inline assembly (even when emitting assembly output). UseIntegratedAs is set to true for targets that consider any failure to parse valid assembly to be a bug. Target specific subclasses generally enable the integrated assembler in their constructor. The default value can be overridden with -no-integrated-as.

All tests that rely on inline assembly supporting invalid assembly (for example, those that use mnemonics such as 'foo' or 'hello world') have been updated to disable the integrated assembler.

Diff Detail

Event Timeline

dsanders updated this revision to Unknown Object (????).Feb 4 2014, 6:12 AM

Use llvm::Triple::getArch() instead of matching triples directly.

dsanders updated this revision to Unknown Object (????).Feb 4 2014, 6:27 AM

Minor tweaks

dsanders added a subscriber: Unknown Object (MLST).Feb 4 2014, 6:28 AM

Here's my patch, but there's a problem with it.

As-is, it causes ~50 test failures. The ones I've looked at so far are generally due to directives such as .foo_directive or .bork_directive in inline assembly. I could go through and change each test to use valid assembly, but I think a better solution would be to have isIntegratedAsDefault() and useIntegratedAs() as the predicates and then have an equivalent to -no-integrated-as force useIntegratedAs() to return false. This is the same model as clang uses.

The effective change to the plan we previously discussed is that there is a way to force the use of EmitRawText() using -no-integrated-as. The default behaviour where targets with mature MC's always parse inline assembly remains the same.

dsanders updated this revision to Unknown Object (????).Feb 4 2014, 9:06 AM

Switched to isIntegratedAsDefault() and useIntegratedAs() method.
Added -no-integrated-inline-as to llc and used it where tests required invalid
assembly, magic comments, or non-canonical output.

This is certainly going on the right direction. Thank you so much for working on this!

include/llvm/MC/MCSubtargetInfo.h
48 ↗(On Diff #6854)

Why MCSubtargeInfo? I have the impression that MCAsmInfo might be a better match. Its initialization structure is a bit more verbose, but being split among the targets matches what clang does a bit better and should allow each target to change the rules for when they use the integrated assembler without touching common code.

74 ↗(On Diff #6854)

Don't duplicate the name on the comment.

lib/MC/MCSubtargetInfo.cpp
22 ↗(On Diff #6854)

If going with MCasmInfo, I would suggest adding a set method to it. That way the constructor set the default and LLVMTargetMachine::initAsmInfo can call the setter if -no-integrated-inline-as is passed.

136 ↗(On Diff #6854)

The clang logic has more cases. For example, I think the ppc assembler is used on some BSDs.
This is probably fine for now. Just change the FIXME to say that this version should be synced with clang and clang changed to use this so that we have a single implementation.

Thanks for the review.

In addition to the changes suggested by comments, I'm going to drop the word 'Inline' from the variables and options. Otherwise it will be misleading when clang/llvm's logic is unified.

include/llvm/MC/MCSubtargetInfo.h
48 ↗(On Diff #6854)

I was thinking that the maturity of MC support potentially varies on a subtarget basis. For example, if MC support for MIPS32r2 is mature, it doesn't follow that MC support for MIPS-I is also mature.

Having looked at MCAsmInfo, I agree that it's a better match and should still be able to handle any likely subtarget issues. It also has the nice benefit of keeping the targets separate in isIntegratedInlineAsDefault() since it's normally subclassed.

74 ↗(On Diff #6854)

I did this because every other function in the file was doing the same thing. I'll remove it.

lib/MC/MCSubtargetInfo.cpp
136 ↗(On Diff #6854)

Yes you're right. I think I've found all of them now.

dsanders updated this revision to Unknown Object (????).Feb 6 2014, 2:54 AM
  • Moved the predicates to MCAsmInfo.
  • Implemented all (except one) of clangs conditions for whether -integrated-as is the default.
    • A couple conditions have imperfect conversions. For example testing for MachO output has become testing for Darwin.
    • getOS() == Solaris, is currently missing. There doesn't seem to be a good place to put it.
  • Dropped the word 'Inline' from variables and option names. If this code is going to be unified with clang's logic then it will cover inline and non-inline asm.

99% there.

Sorry for the delay with code review. The only real issue is the question about having two member variables instead of one.

include/llvm/MC/MCAsmInfo.h
309

Why do we need two variables? It seems that the constructors should be able to set UseIntegratedAssembler directly and LLVMTargetMachine.cpp can then override it if requested.

lib/MC/MCAsmInfo.cpp
96

IMHO it is the MCAsmInfoDarwin that should be named MCAsmInfoMachO. The handling of COFF and MachO are probably correct enough to not need a FIXME :-)

test/CodeGen/Generic/mature-mc-support.ll
1

nit: add something like "is parsed even when printing assembly".

dsanders added inline comments.Feb 10 2014, 9:41 AM
include/llvm/MC/MCAsmInfo.h
309

I was mostly following clang's lead for this. Clang doesn't seem to use the separation for much (the only bit I've found is that the default value is re-used as the default for -masm-verbose) so I'll change it to a single variable.

lib/MC/MCAsmInfo.cpp
96

Great. Do you have any thoughts on the 'OS is Solaris' check?

I think I can put it in SparcELFMCAsmInfo and X86ELFMCAsmInfo to cover most of it, but I'm not sure if there are other subclasses that need it.

IMHO it is the MCAsmInfoDarwin that should be named MCAsmInfoMachO. The handling of COFF and MachO are probably correct enough to not need a FIXME :-)

Great. Do you have any thoughts on the 'OS is Solaris' check?

I think I can put it in SparcELFMCAsmInfo and X86ELFMCAsmInfo to cover most of it, but I'm not sure if there are other subclasses that need it.

That should do it. If people want in the future we can move it up to
MCAsmInfoELF by passing a triple down to the constructor.

Cheers,
Rafael

dsanders updated this revision to Unknown Object (????).Feb 11 2014, 7:13 AM
  • Removed IsIntegratedAssemblerDefault.
  • Added missing 'OS is Solaris' check.
  • Fixed nit.
rafael accepted this revision.Feb 11 2014, 11:32 AM

LGTM!

Thanks!

dsanders closed this revision.Feb 12 2014, 6:51 AM
dsanders closed this revision.

Closed by commit rL201237 (authored by @dsanders).