Page MenuHomePhabricator

[Mips] Use appropriate private label prefix based on Mips ABI
Needs ReviewPublic

Authored by mbrkusanin on Tue, Aug 27, 3:40 AM.

Details

Summary

MipsMCAsmInfo was using '$' prefix for Mips32 and '.L' for Mips64 regardless of
-target-abi option. By passing MCTargetOptions to MCAsmInfo we can find out Mips
ABI and pick appropriate prefix.

Diff Detail

Event Timeline

mbrkusanin created this revision.Tue, Aug 27, 3:40 AM
  • adjustPrefixes() is not called after every call of createMCAsmInfo(), only where MCTargetOptions is available since this is what tells us if ABI is given as argument.
  • StringRef CPU could be removed from adjustPrefixes() since MipsABIInfo::computeTargetABI() does not use it in it's implementation and an empty string could be passed. We could remove Triple as well and use MCTargetOptions.ABIName only and read it manualy, but this way we would keep it consistent with other uses of MipsABIInfo.
  • adjustPrefixes() is not called after every call of createMCAsmInfo(), only where MCTargetOptions is available since this is what tells us if ABI is given as argument.
  • StringRef CPU could be removed from adjustPrefixes() since MipsABIInfo::computeTargetABI() does not use it in it's implementation and an empty string could be passed. We could remove Triple as well and use MCTargetOptions.ABIName only and read it manualy, but this way we would keep it consistent with other uses of MipsABIInfo.

We've also had some issues with the MIPS ABI not being available in the MC* classes for CHERI.
Would it be possible to require a const MCTargetOptions &Options when creating MCAsmInfo?
If MCTargetOptions isn't always available, how about passing an Optional<MCTargetOptions> to MCAsmInfo? That would be equivalent to the current approach but make it harder to forget calling adjustPrefixes()

Would it be possible to require a const MCTargetOptions &Options when creating MCAsmInfo?
If MCTargetOptions isn't always available, how about passing an Optional<MCTargetOptions> to MCAsmInfo? That would be equivalent to the current approach but make it harder to forget calling adjustPrefixes()

That would look something like this https://reviews.llvm.org/differential/diff/217420/ . I've already tested this solution earlier (except using Optional<>) but decided against it because of changes to MCAsmInfo for every target. Yes, the advantage is that it would be harder to forget about 'adjustPrefixes()' vs some more changes in code. Let me know which way you prefer.

Would it be possible to require a const MCTargetOptions &Options when creating MCAsmInfo?
If MCTargetOptions isn't always available, how about passing an Optional<MCTargetOptions> to MCAsmInfo? That would be equivalent to the current approach but make it harder to forget calling adjustPrefixes()

That would look something like this https://reviews.llvm.org/differential/diff/217420/ . I've already tested this solution earlier (except using Optional<>) but decided against it because of changes to MCAsmInfo for every target. Yes, the advantage is that it would be harder to forget about 'adjustPrefixes()' vs some more changes in code. Let me know which way you prefer.

I forgot that MCTargetOptions will be a pointer so using nullptr is probably easier than Optional.

Personally, I would prefer always passing MCTargetOptions to MCAsmInfo since it will allow us to use MipsABIInfo to set appropriate values for CHERI-MIPS.
If you remove the default argument from createMCAsmInfo, how many files need to explicitly pass nullptr/None?

mbrkusanin updated this revision to Diff 217653.EditedWed, Aug 28, 8:04 AM
mbrkusanin edited the summary of this revision. (Show Details)
  • Now passing const MCTargetOptions * to MCAsmInfo().
  • Removed adjustPrefix().
  • Modified MCAsmInfo() for each target to accept new parameters.

If you remove the default argument from createMCAsmInfo, how many files need to explicitly pass nullptr/None?

In LLVM there are 13 files and also a couple more in Clang:

Search results in case you're interested.

llvm/lib/MC/MCDisassembler/Disassembler.cpp:60
std::unique_ptr<const MCAsmInfo> MAI(TheTarget->createMCAsmInfo(*MRI, TT));
llvm/lib/Object/ModuleSymbolTable.cpp:86
std::unique_ptr<MCAsmInfo> MAI(T->createMCAsmInfo(*MRI, TT.str()));
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp:390
AsmInfo.reset(ObjectTarget->createMCAsmInfo(*RegisterInfo, TripleName));
llvm/tools/sancov/sancov.cpp:818
std::unique_ptr<const MCAsmInfo> AsmInfo(
TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp:726
std::unique_ptr<MCAsmInfo> MAI(TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/tools/llvm-objdump/llvm-objdump.cpp:1506
std::unique_ptr<const MCAsmInfo> AsmInfo(
TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/tools/llvm-objdump/MachODump.cpp:7215:
std::unique_ptr<const MCAsmInfo> AsmInfo(
TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/tools/llvm-objdump/MachODump.cpp:7265:
ThumbAsmInfo.reset(
ThumbTarget->createMCAsmInfo(*ThumbMRI, ThumbTripleName));
llvm/tools/llvm-mca/llvm-mca.cpp:351
std::unique_ptr<MCAsmInfo> MAI(TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/tools/llvm-exegesis/lib/Analysis.cpp:174
AsmInfo_.reset(Target.createMCAsmInfo(*RegInfo_, FirstPoint.LLVMTriple));
llvm/tools/llvm-jitlink/llvm-jitlink.cpp:526
std::unique_ptr<MCAsmInfo> MAI(TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/unittests/ExecutionEngine/JITLink/JITLinkTestCommon.cpp:62
MAI.reset(TheTarget->createMCAsmInfo(*MRI, TT.getTriple()));
llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp:412
MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName));
llvm/unittests/MC/DwarfLineTables.cpp:40
MAI.reset(TheTarget->createMCAsmInfo(*MRI, Triple));

I would prefer always passing MCTargetOptions to MCAsmInfo too. Could you try this approach? But take a look at LLVMCreateDisasmCPUFeatures function from Disassembler.cpp. If we cannot retrieve MCTargetOptions right in this function, we will have to change "LLVM-C" interface LLVMCreateDisasmCPUFeatures function. There is an interesting comment in the D20916 - "Why not use MCTargetOptions::ABIName".