Page MenuHomePhabricator

[Mips] Use appropriate private label prefix based on Mips ABI
ClosedPublic

Authored by mbrkusanin on Aug 27 2019, 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.Aug 27 2019, 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.EditedAug 28 2019, 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".

mbrkusanin updated this revision to Diff 220675.EditedSep 18 2019, 8:14 AM
  • MCTargetOptions is now always passed to MCAsmInfo (or rather createMCAsmInfo). In places where we it wasn't available before we simply pass empty MCTargetOptions.
  • Patch is now for monorepo. Besides LLVM there are now changes to Clang and LLDB. I needed to use monorepo to test if everything builds correctly. If this is accepted I will split it into separate patches if needed.
  • Also it might be best to split this into two changes. First one that adds MCTargetOptions to MCAsmInfo and second one that fixes prefix for Mips which is what I set out to solve.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 18 2019, 8:14 AM

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.

As far as I can tell there is currently no way to specify ABI from C interface. So I just made empty MCTargetOptions. Same with other cases where it was needed. Some places like llvm-mca.cpp had required options available so there we could initialize MCTargetOptions.

arichardson accepted this revision.Sep 18 2019, 10:45 AM

Looks good to me but I guess someone else should give the final approval.

llvm/lib/Target/Mips/MCTargetDesc/MipsMCAsmInfo.cpp
25–27

Unrelated to this diff, but I guess this Triple environment check should be moved down and use ABI.isN32()

This revision is now accepted and ready to land.Sep 18 2019, 10:45 AM
  • MipsMCAsmInfo() now always reads ABI from MipsABIInfo instead of Triple.
mbrkusanin marked an inline comment as done.Sep 19 2019, 2:49 AM
mbrkusanin added a comment.EditedSep 19 2019, 3:03 AM

Any comment on whether we should split this into two patches? One that adds MCTargetOptions to MCAsmInfo and another one that just fixes prefixes for Mips?

atanasyan accepted this revision.Sep 24 2019, 9:38 AM

LGTM. But before commit get more approvals. For example, from echristo, code owners of other targets, etc.

I would keep the as-is. In that case a target of such huge modifications looks a bit more clear.

This makes sense to me (although we don't currently need the options parameter there).

  • Rebase
  • Ping

@echristo @craig.topper @tstellar @dylanmckay @petecoup
If there are no objections then I'll split this into llvm, clang and lldb patches and commit them next week.

@echristo @craig.topper @tstellar @dylanmckay @petecoup
If there are no objections then I'll split this into llvm, clang and lldb patches and commit them next week.

Mirko, I think you can commit the patch now because we have not got any objections.

I kinda want the option to be encoded in the triple for earlier testing of linking issues, but for now this is probably OK.

This revision was automatically updated to reflect the committed changes.