Page MenuHomePhabricator

Thread MCSubtargetInfo through Target::createMCAsmBackend
ClosedPublic

Authored by asb on Dec 18 2017, 5:33 AM.

Details

Summary

Currently it's not possible to access MCSubtargetInfo from a TgtMCAsmBackend. D20830 threaded an MCSubtargetInfo reference through MCAsmBackend::relaxInstruction, but this isn't the only function that would benefit from access. This patch removes the Triple and CPUString arguments from createMCAsmBackend and replaces them with MCSubtargetInfo.

This patch just changes the interface without making any intentional functional changes. Once in, several cleanups are possible:

  • Get rid of the awkward MCSubtargetInfo handling in ARMAsmBackend
  • Support 16-bit instructions when valid in MipsAsmBackend::writeNopData
  • Get rid of the CPU string parsing in X86AsmBackend and just use a SubtargetFeature for HasNopl
  • Emit 16-bit nops in RISCVAsmBackend::writeNopData if the compressed instruction set extension is enabled (see D41221)

I'm not sure who to tag as reviewer, but given this enables useful ARM and X86 cleanups I'm tagging code owners for those targets. Also Lang Hames and Daniel Sanders for touching TargetRegistry recently. Feel free to add yourself as a reviewer to tag anyone else you think would be appropriate.

X86 maintainers: This currently causes some test failures related to the X86 HasNopl code which looks to me like I'm just exposing an existing bug. HasNopl is false if the CPU string is "generic", "i386", and a bunch of others but true for the empty string. This means -triple=i686-pc-linux-gnu allows long nops but -triple=i686-pc-linux-gnu -mcpu=generic doesn't, which seems wrong. I've filed https://bugs.llvm.org/show_bug.cgi?id=35686

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Dec 18 2017, 5:33 AM
fhahn added a subscriber: fhahn.Dec 18 2017, 5:34 AM
fhahn added a comment.Dec 18 2017, 5:44 AM

Get rid of the awkward MCSubtargetInfo handling in ARMAsmBackend

Yeah, not having to re-create MCSubtargetInfo in ARMMCTargetDesc.cpp would be great I think!

asb edited the summary of this revision. (Show Details)Dec 18 2017, 6:10 AM

I've filed the X86 HasNopl issue here: https://bugs.llvm.org/show_bug.cgi?id=35686

This will also be very useful for CHERI clang. It will allow me to remove some hacks I had to add in order to get information from the subtarget in the assembler backend.

I agree the current behavior of empty CPU string for nopl is a bug.

We should also remove the string compares for max nop length for silvermont after this too.

asb added a comment.Dec 19 2017, 2:34 AM

It hasn't made it through to Phabricator for some reason, but Rafael gave a LGTM on the on the commits list: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171218/511408.html

The X86 HasNopl issue was fixed in rL321026.

@echristo: did you have any comments?

asb added a comment.Jan 2 2018, 12:13 AM

Ping? This patch still applies against current HEAD and is ready to commit from my perspective. Does it look good to you?

fhahn accepted this revision.Jan 2 2018, 3:31 AM

LGTM. Rafael also LGTM on llvm-commits, but I think it's worth holding off committing another day, to give people a chance to voice additional concerns.

This revision is now accepted and ready to land.Jan 2 2018, 3:31 AM
echristo accepted this revision.Jan 2 2018, 4:55 PM

LGTM.

-eric

This revision was automatically updated to reflect the committed changes.
asb added a comment.Jan 3 2018, 2:57 AM

Thanks for the reviews everyone. Does anyone have any objections to removing the now seemingly redundant MCSubtargetInfo argument to MCAsmBackend::relaxInstruction?

D41693 refactors the code I mentioned in ArmAsmBackend. I'm not currently planning to write the patch for X86 HasNopl, but that issue is tracked here. No plans for Mips nop insertion either, but that should now be easy to support.