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.
Details
Diff Detail
Event Timeline
- 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()
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?
- Now passing const MCTargetOptions * to MCAsmInfo().
- Removed adjustPrefix().
- Modified MCAsmInfo() for each target to accept new parameters.
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".
- 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.
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.
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() |
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?
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.
@echristo @craig.topper @uweigand @tstellar @dylanmckay @petecoup
Do you have any comments on the current patch?
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.
I kinda want the option to be encoded in the triple for earlier testing of linking issues, but for now this is probably OK.
Unrelated to this diff, but I guess this Triple environment check should be moved down and use ABI.isN32()