This is an archive of the discontinued LLVM Phabricator instance.

[MC] Use local MCSubtargetInfo in writeNops
ClosedPublic

Authored by peter.smith on Apr 23 2018, 7:28 AM.

Details

Summary

On some architectures such as Arm and X86 the encoding for a nop may change depending on the subtarget in operation at the time of encoding. This change replaces the per module MCSubtargetInfo retained by the targets AsmBackend in favour of passing through the local MCSubtargetInfo in operation at the time.

On Arm using the architectural NOP instruction can have a performance benefit on some implementations.

For Arm I've deleted the copy of the AsmBackend's MCSubtargetInfo to limit the chances of this causing problems in the future. I've not done this for other targets such as X86 as there is more frequent use of the MCSubtargetInfo and it looks to be for stable properties that we would not expect to vary per function in the way that an Arm NOP might.

I've attempted to take into account the in tree experimental backends.

This patch depends on D45961

Diff Detail

Event Timeline

peter.smith created this revision.Apr 23 2018, 7:28 AM

Rebased [NFC]

nhaehnle removed a subscriber: nhaehnle.Jun 4 2018, 7:18 AM

Rebased, and added support for WebAssembly and RiscV as my default build configuration didn't include them at the time. No other changes.

efriedma accepted this revision.Jul 29 2021, 2:53 PM
efriedma added a subscriber: efriedma.

LGTM, assuming it rebases cleanly

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h
22 ↗(On Diff #173707)

isThumbMode should probably be using the ThumbMode bit of MCSubtargetInfo... but I'm not sure we're actually updating that at the moment. Probably okay to leave it for a followup.

This revision is now accepted and ready to land.Jul 29 2021, 2:53 PM
MaskRay added inline comments.Jul 29 2021, 3:26 PM
lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
348 ↗(On Diff #173707)

STI->

Thanks for the review. I've just got back from vacation and I'm a bit behind. Will rebase this and D45961 hopefully by the end of this week. I'll post it here and will give a heads up if there are any meaningful changes.

Thanks for the review. I've just got back from vacation and I'm a bit behind. Will rebase this and D45961 hopefully by the end of this week. I'll post it here and will give a heads up if there are any meaningful changes.

Will be early next week. I've rebased D45962 which this depends on.

Thanks for the review. I've just got back from vacation and I'm a bit behind. Will rebase this and D45961 hopefully by the end of this week. I'll post it here and will give a heads up if there are any meaningful changes.

Will be early next week. I've rebased D45962 which this depends on.

I've just had a quick go at rebasing and there is a bit more work than I bargained for. The X86 backend has introduced a couple more MCFragments that will need adapting

  • MCBoundaryAlignFragment
  • MCNopsFragment

I think I can pass STI through to these wihtout any ill effect but I'd like to submit these as individual patches for review as I don't know X86 well.

The X86 backend has used its per-target STI for some other things as well, so I won't be able to remove it in this patch.

ETA for this will be Friday afternoon (doing this in spare time).

peter.smith edited the summary of this revision. (Show Details)

Rebased (quite a lot) to account for a couple of years of changes.

I've included the changes to the two new x86 specific MCFragments MCNopsFragment and MCBoundaryAlignFragment that also call emitNops. I can split these changes out if needed but I thought it better to show the magnitude of the change in one go.

As mentioned in D45961 an alternative, less disruptive implementation (the Arm backend does not use MCNopsFragment or MCBoundaryAlignFragment) is to permit the MCSubtargetInfo to be nullptr which would mean retaining the STI in the Arm MCAsmBackend and a test in emitNops() to see if the STI passed in where nullptr.

Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 9:43 AM

Minor rebase NFC from previous patch. As this + previous rebase was quite a substantial update from the previous patch I'd ideally like a second opinion on whether this is still OK before I commit it and D45961 .

bcain added a subscriber: bcain.Sep 3 2021, 10:14 AM
MaskRay accepted this revision.Sep 4 2021, 9:48 AM

LGTM.

Rebase, and fix up DWARFExpressionCopyBytesTest.cpp

Fixup comments in MCFragment.h to look like D45961 and update patch manually as I'm not sure arc diff did the right thing.

MaskRay accepted this revision.Sep 6 2021, 9:41 AM
MaskRay added inline comments.
llvm/test/CodeGen/ARM/subtarget-align.ll
1

< and -o - can be omitted.

llvm/test/MC/ARM/subtarget-nop.s
1

< and -o - can be omitted.

Thanks will apply comments prior to commit.

This revision was landed with ongoing or failed builds.Sep 7 2021, 7:47 AM
This revision was automatically updated to reflect the committed changes.

This broke llvm-ml for assembling real-world files:

$ bin/llvm-ml -D_M_IA32 -DOMPT_SUPPORT=0 -c -Fo out.obj ../../openmp/runtime/src/z_Windows_NT-586_asm.asm
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/llvm-ml -D_M_IA32 -DOMPT_SUPPORT=0 -c -Fo out.obj ../../openmp/runtime/src/z_Windows_NT-586_asm.asm
 #0 0x000055f22ac9ab1c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:565:0
 #1 0x000055f22ac9abd3 PrintStackTraceSignalHandler(void*) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:632:0
 #2 0x000055f22ac98887 llvm::sys::RunSignalHandlers() /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Signals.cpp:97:0
 #3 0x000055f22ac9a49d SignalHandler(int) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:407:0
 #4 0x00007f0ff42e3980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x000055f22a777efc llvm::FeatureBitset::operator[](unsigned int) const /home/martin/code/llvm-project/llvm/build-debug/../include/llvm/MC/SubtargetFeature.h:87:0
 #6 0x000055f22a920116 (anonymous namespace)::X86AsmBackend::writeNopData(llvm::raw_ostream&, unsigned long, llvm::MCSubtargetInfo const*) const /home/martin/code/llvm-project/llvm/build-debug/../lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1145:0
 #7 0x000055f22a9fb58b writeFragment(llvm::raw_ostream&, llvm::MCAssembler const&, llvm::MCAsmLayout const&, llvm::MCFragment const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:548:0
 #8 0x000055f22a9fc778 llvm::MCAssembler::writeSectionData(llvm::raw_ostream&, llvm::MCSection const*, llvm::MCAsmLayout const&) const /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:780:0
 #9 0x000055f22aad17fd (anonymous namespace)::WinCOFFObjectWriter::writeSectionContents(llvm::MCAssembler&, llvm::MCAsmLayout const&, llvm::MCSection const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:614:0
#10 0x000055f22aad1965 (anonymous namespace)::WinCOFFObjectWriter::writeSection(llvm::MCAssembler&, llvm::MCAsmLayout const&, (anonymous namespace)::COFFSection const&, llvm::MCSection const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:635:0
#11 0x000055f22aad4363 (anonymous namespace)::WinCOFFObjectWriter::writeObject(llvm::MCAssembler&, llvm::MCAsmLayout const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:1152:0
#12 0x000055f22a9fd579 llvm::MCAssembler::Finish() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:937:0
#13 0x000055f22aa68b5e llvm::MCObjectStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCObjectStreamer.cpp:888:0
#14 0x000055f22aa9eaca llvm::MCWinCOFFStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCWinCOFFStreamer.cpp:368:0
#15 0x000055f22a916966 (anonymous namespace)::X86WinCOFFStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/Target/X86/MCTargetDesc/X86WinCOFFStreamer.cpp:65:0
#16 0x000055f22aa89d3c llvm::MCStreamer::Finish(llvm::SMLoc) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCStreamer.cpp:1005:0
#17 0x000055f22ab53df0 (anonymous namespace)::MasmParser::Run(bool, bool) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCParser/MasmParser.cpp:1435:0
#18 0x000055f22a701374 AssembleInput(llvm::StringRef, llvm::Target const*, llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo&, llvm::MCSubtargetInfo&, llvm::MCInstrInfo&, llvm::MCTargetOptions&, llvm::opt::ArgList const&) /home/martin/code/llvm-project/llvm/build-debug/../tools/llvm-ml/llvm-ml.cpp:186:0
#19 0x000055f22a703135 main /home/martin/code/llvm-project/llvm/build-debug/../tools/llvm-ml/llvm-ml.cpp:433:0

@epastor Apparently it would be good with a llvm-ml specific testcase that triggers writing nops.

This broke llvm-ml for assembling real-world files:

$ bin/llvm-ml -D_M_IA32 -DOMPT_SUPPORT=0 -c -Fo out.obj ../../openmp/runtime/src/z_Windows_NT-586_asm.asm
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/llvm-ml -D_M_IA32 -DOMPT_SUPPORT=0 -c -Fo out.obj ../../openmp/runtime/src/z_Windows_NT-586_asm.asm
 #0 0x000055f22ac9ab1c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:565:0
 #1 0x000055f22ac9abd3 PrintStackTraceSignalHandler(void*) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:632:0
 #2 0x000055f22ac98887 llvm::sys::RunSignalHandlers() /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Signals.cpp:97:0
 #3 0x000055f22ac9a49d SignalHandler(int) /home/martin/code/llvm-project/llvm/build-debug/../lib/Support/Unix/Signals.inc:407:0
 #4 0x00007f0ff42e3980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x000055f22a777efc llvm::FeatureBitset::operator[](unsigned int) const /home/martin/code/llvm-project/llvm/build-debug/../include/llvm/MC/SubtargetFeature.h:87:0
 #6 0x000055f22a920116 (anonymous namespace)::X86AsmBackend::writeNopData(llvm::raw_ostream&, unsigned long, llvm::MCSubtargetInfo const*) const /home/martin/code/llvm-project/llvm/build-debug/../lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1145:0
 #7 0x000055f22a9fb58b writeFragment(llvm::raw_ostream&, llvm::MCAssembler const&, llvm::MCAsmLayout const&, llvm::MCFragment const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:548:0
 #8 0x000055f22a9fc778 llvm::MCAssembler::writeSectionData(llvm::raw_ostream&, llvm::MCSection const*, llvm::MCAsmLayout const&) const /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:780:0
 #9 0x000055f22aad17fd (anonymous namespace)::WinCOFFObjectWriter::writeSectionContents(llvm::MCAssembler&, llvm::MCAsmLayout const&, llvm::MCSection const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:614:0
#10 0x000055f22aad1965 (anonymous namespace)::WinCOFFObjectWriter::writeSection(llvm::MCAssembler&, llvm::MCAsmLayout const&, (anonymous namespace)::COFFSection const&, llvm::MCSection const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:635:0
#11 0x000055f22aad4363 (anonymous namespace)::WinCOFFObjectWriter::writeObject(llvm::MCAssembler&, llvm::MCAsmLayout const&) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/WinCOFFObjectWriter.cpp:1152:0
#12 0x000055f22a9fd579 llvm::MCAssembler::Finish() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCAssembler.cpp:937:0
#13 0x000055f22aa68b5e llvm::MCObjectStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCObjectStreamer.cpp:888:0
#14 0x000055f22aa9eaca llvm::MCWinCOFFStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCWinCOFFStreamer.cpp:368:0
#15 0x000055f22a916966 (anonymous namespace)::X86WinCOFFStreamer::finishImpl() /home/martin/code/llvm-project/llvm/build-debug/../lib/Target/X86/MCTargetDesc/X86WinCOFFStreamer.cpp:65:0
#16 0x000055f22aa89d3c llvm::MCStreamer::Finish(llvm::SMLoc) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCStreamer.cpp:1005:0
#17 0x000055f22ab53df0 (anonymous namespace)::MasmParser::Run(bool, bool) /home/martin/code/llvm-project/llvm/build-debug/../lib/MC/MCParser/MasmParser.cpp:1435:0
#18 0x000055f22a701374 AssembleInput(llvm::StringRef, llvm::Target const*, llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo&, llvm::MCSubtargetInfo&, llvm::MCInstrInfo&, llvm::MCTargetOptions&, llvm::opt::ArgList const&) /home/martin/code/llvm-project/llvm/build-debug/../tools/llvm-ml/llvm-ml.cpp:186:0
#19 0x000055f22a703135 main /home/martin/code/llvm-project/llvm/build-debug/../tools/llvm-ml/llvm-ml.cpp:433:0

@epastor Apparently it would be good with a llvm-ml specific testcase that triggers writing nops.

My apologies, will take a look. If I can spot the problem early I'll fix, otherwise will need to revert as it may be Friday before I can spend a lot of time on it.

Created D109425 to fix. This is the smallest change I can make in the time I've got this morning. Looks like this only got through because of an unfortunate default parameter. We might be able to prevent that by changing to a const reference.