[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
flip1995 updated this revision to Diff 342345.
flip1995 added a comment.
[MC] Untangle MCContext and MCObjectFileInfo
This untangles the MCContext and the MCObjectFileInfo. There is a circular
dependency between MCContext and MCObjectFileInfo. Currently this dependency
also exists during construction: You can't contruct a MOFI without a MCContext
without constructing the MCContext with a dummy version of that MOFI first.
This removes this dependency during construction. In a perfect world,
MCObjectFileInfo wouldn't depend on MCContext at all, but only be stored in the
MCContext, like other MC information. This is future work.
This also shifts/adds more information to the MCContext making it more
available to the different targets. Namely:
- TargetTriple
- ObjectFileType
- SubtargetInfo
Included Commits:
- [MC] Add MCSubtargetInfo to MCContext
- [MC] Untangle MCContext and MCObjectFileInfo
- [MC] Move TargetTriple information from MCObjectFileInfo to MCContext
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101462/new/
https://reviews.llvm.org/D101462
Files:
clang/lib/Parse/ParseStmtAsm.cpp
clang/tools/driver/cc1as_main.cpp
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
llvm/include/llvm/MC/MCContext.h
llvm/include/llvm/MC/MCObjectFileInfo.h
llvm/lib/CodeGen/MachineModuleInfo.cpp
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
llvm/lib/DWARFLinker/DWARFStreamer.cpp
llvm/lib/MC/MCAsmStreamer.cpp
llvm/lib/MC/MCContext.cpp
llvm/lib/MC/MCDisassembler/Disassembler.cpp
llvm/lib/MC/MCMachOStreamer.cpp
llvm/lib/MC/MCObjectFileInfo.cpp
llvm/lib/MC/MCParser/AsmParser.cpp
llvm/lib/MC/MCParser/COFFAsmParser.cpp
llvm/lib/MC/MCParser/DarwinAsmParser.cpp
llvm/lib/MC/MCParser/MasmParser.cpp
llvm/lib/MC/MCStreamer.cpp
llvm/lib/MC/MCWinCOFFStreamer.cpp
llvm/lib/Object/ModuleSymbolTable.cpp
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp
llvm/lib/Target/TargetLoweringObjectFile.cpp
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
llvm/tools/llvm-dwp/llvm-dwp.cpp
llvm/tools/llvm-exegesis/lib/Analysis.cpp
llvm/tools/llvm-exegesis/lib/LlvmState.cpp
llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
llvm/tools/llvm-mc-assemble-fuzzer/llvm-mc-assemble-fuzzer.cpp
llvm/tools/llvm-mc/llvm-mc.cpp
llvm/tools/llvm-mca/llvm-mca.cpp
llvm/tools/llvm-ml/Disassembler.cpp
llvm/tools/llvm-ml/llvm-ml.cpp
llvm/tools/llvm-objdump/MachODump.cpp
llvm/tools/llvm-objdump/llvm-objdump.cpp
llvm/tools/llvm-profgen/ProfiledBinary.cpp
llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp
llvm/tools/sancov/sancov.cpp
llvm/unittests/CodeGen/MachineInstrTest.cpp
llvm/unittests/CodeGen/MachineOperandTest.cpp
llvm/unittests/CodeGen/TestAsmPrinter.cpp
llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp
llvm/unittests/MC/DwarfLineTables.cpp
llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
Index: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
===
--- llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
+++ llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
@@ -57,6 +57,7 @@
std::unique_ptr MRI;
std::unique_ptr MUPMAI;
std::unique_ptr MII;
+ std::unique_ptr MSTI;
std::unique_ptr Str;
std::unique_ptr Parser;
std::unique_ptr Ctx;
@@ -86,6 +87,11 @@
MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCOptions));
EXPECT_NE(MAI, nullptr);
+std::unique_ptr MSTI;
+MSTI.reset(TheTarget->createMCSubtargetInfo(TripleName, /*CPU=*/"",
+/*Features=*/""));
+EXPECT_NE(MSTI, nullptr);
+
// Now we cast to our mocked up version of MCAsmInfo.
MUPMAI.reset(static_cast(MAI.release()));
// MUPMAI should "hold" MAI.
@@ -99,9 +105,9 @@
SrcMgr.AddNewSourceBuffer(std::move(Buffer), SMLoc());
EXPECT_EQ(Buffer, nullptr);
-Ctx.reset(
-new MCContext(MUPMAI.get(), MRI.get(), , , ));
-MOFI.InitMCObjectFileInfo(Triple, false, *Ctx, false);
+Ctx.reset(new MCContext(Triple, MUPMAI.get(), MRI.get(), , MSTI.get(),
+, ));
+MOFI.InitMCObjectFileInfo(/*PIC=*/false, *Ctx, /*LargeCodeModel=*/false);
Str.reset(TheTarget->createNullStreamer(*Ctx));
Index: llvm/unittests/MC/DwarfLineTables.cpp
===
--- llvm/unittests/MC/DwarfLineTables.cpp
+++ llvm/unittests/MC/DwarfLineTables.cpp
@@ -21,7 +21,7 @@
namespace {
struct Context {
- const char *Triple = "x86_64-pc-linux";
+ const char *TripleName = "x86_64-pc-linux";
std::unique_ptr MRI;
std::unique_ptr MAI;
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
shafik added inline comments. Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:909 + new llvm::MCContext(llvm::Triple(triple), asm_info_up.get(), + reg_info_up.get(), nullptr, subtarget_info_up.get())); if (!context_up) Please add comments with the names of parameters for constants as documented here: https://llvm.org/docs/CodingStandards.html#comment-formatting ``` new llvm::MCContext(llvm::Triple(triple), asm_info_up.get(), reg_info_up.get(), /*MOFI=*/nullptr, subtarget_info_up.get())); ``` This applies to a bunch of places, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
shafik added inline comments. Comment at: clang/lib/Parse/ParseStmtAsm.cpp:589 + TheTarget->createMCObjectFileInfo( + /*PIC*/ false, Ctx)); + Ctx.setObjectFileInfo(MOFI.get()); `/*PIC=*/false` Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:381 + std::unique_ptr MOFI(TheTarget->createMCObjectFileInfo( + /* PIC= */ false, Ctx)); + Ctx.setObjectFileInfo(MOFI.get()); `/*PIC=*/ false` see clang-tidy rule that checks for this format: https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
flip1995 added a comment. > The refactoring adding `Triple` to `MCContext::MCContext` [...] should be > separate. In order to make the `MCContext` construction independent from the `MCObjectFileInfo`, passing the `Triple` to the `MCContext` is necessary anyway. Moving it completely to the `MCContext` was just the logical next step for me. It also simplifies calls across the code base since it removes an indirection over `MOFI` when accessing the triple. I'll update this on Monday Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
MaskRay added a comment. The `InitMCObjectFileInfo` refactoring is good, but I'll suggest `initMCObjectFileInfo(MCContext , bool PIC, bool LargeCodeModel)` (change the function name to `lowerCase`, and reorder MCContext before bool arguments). The refactoring adding `Triple` to MCContext::MCContext should be separate. We sill want to see whether that is good. The RISC-V functionality change should be separate as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
flip1995 added a comment. Thanks for the review! I already thought that I will have to split this up, so I made the commits self contained so I'll do that. One question before I start: Where should I split this? Should I only split out the RISC-V patch and leave the changes that targets can define their own `MCObjectFileInfo` in (commit a1b3a604a410), or should I also split out that part? Personally I would keep this change in the refactor PR and only split out the RISC-V changes. --- About the alignment change: I guess we can discuss this then specifically for RISC-V in the split out review. But the short answer is: The main motivation is improving code size a bit, since less padding has to be added between functions. If function alignment is important `-align-all-functions=X` could be used. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
MaskRay added a comment. The refactoring part seems useful on its own. The controversial 4-byte alignment part should be split. Doesn't GNU as use 4 for most architectures as well? Why do you want to change it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
jrtc27 added a comment. Herald added a subscriber: JDevlieghere. What's the benefit of less-aligned functions? Is that not more likely to get poor performance due to cache line straddling of the first instruction? Regardless, the functional change should be separated out from the refactoring. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101462/new/ https://reviews.llvm.org/D101462 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo
flip1995 created this revision. flip1995 added reviewers: MaskRay, rnk, asb. Herald added subscribers: frasercrmck, dexonsmith, luismarques, apazos, sameer.abuasal, s.egerton, Jim, jocewei, rupprecht, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, atanasyan, edward-jones, zzheng, jrtc27, gbedwell, niosHD, sabuasal, simoncook, johnrusso, rbar, hiraditya, mgorny, jholewinski. Herald added a reviewer: andreadb. Herald added a reviewer: jhenderson. flip1995 requested review of this revision. Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, aheejin. Herald added projects: clang, LLDB, LLVM. Some information about the object file may be target specific. As an example, this patch removes the hard-coded 4-byte alignment for text sections and now uses the alignment defined by the target in the target specific MCObjectFileInfo. To get this to work some refactoring was involved: 1. MCObjectFileInfo and MCContext depend on each other (even during construction) 2. This patch removes the dependency of MCContext on MCObjectFileInfo during construction 3. Additionally, it untangles MCContext and MCObjectFileInfo a bit more by moving elements, like the TargetTriple information from MCObjectFileInfo to MCContext. The contruction of a MCContext with a MCObjectFileInfo is still a little weird: 1. Construct MCContext 2. Construct MCObjectFileInfo 3. Set MOFI for MCContext But to untangle this futher, the circular dependency between the two would've to be removed completely, which would be too big of a change for the thing this patch tries to achive. The one backend that now makes usage of this patch is RISC-V, where the text section only has to be 2-byte aligned when the C extension is enabled. This now produces the same alignments for RISC-V text sections as GCC does. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D101462 Files: clang/lib/Parse/ParseStmtAsm.cpp clang/tools/driver/cc1as_main.cpp lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp llvm/include/llvm/MC/MCContext.h llvm/include/llvm/MC/MCObjectFileInfo.h llvm/include/llvm/Support/TargetRegistry.h llvm/lib/CodeGen/MachineModuleInfo.cpp llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp llvm/lib/DWARFLinker/DWARFStreamer.cpp llvm/lib/MC/MCAsmStreamer.cpp llvm/lib/MC/MCContext.cpp llvm/lib/MC/MCDisassembler/Disassembler.cpp llvm/lib/MC/MCELFStreamer.cpp llvm/lib/MC/MCMachOStreamer.cpp llvm/lib/MC/MCObjectFileInfo.cpp llvm/lib/MC/MCParser/AsmParser.cpp llvm/lib/MC/MCParser/COFFAsmParser.cpp llvm/lib/MC/MCParser/DarwinAsmParser.cpp llvm/lib/MC/MCParser/MasmParser.cpp llvm/lib/MC/MCStreamer.cpp llvm/lib/MC/MCWinCOFFStreamer.cpp llvm/lib/Object/ModuleSymbolTable.cpp llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp llvm/lib/Target/RISCV/MCTargetDesc/CMakeLists.txt llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCObjectFileInfo.cpp llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCObjectFileInfo.h llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp llvm/lib/Target/TargetLoweringObjectFile.cpp llvm/test/MC/RISCV/align.s llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp llvm/tools/llvm-dwp/llvm-dwp.cpp llvm/tools/llvm-exegesis/lib/Analysis.cpp llvm/tools/llvm-exegesis/lib/LlvmState.cpp llvm/tools/llvm-exegesis/lib/SnippetFile.cpp llvm/tools/llvm-jitlink/llvm-jitlink.cpp llvm/tools/llvm-mc-assemble-fuzzer/llvm-mc-assemble-fuzzer.cpp llvm/tools/llvm-mc/llvm-mc.cpp llvm/tools/llvm-mca/llvm-mca.cpp llvm/tools/llvm-ml/Disassembler.cpp llvm/tools/llvm-ml/llvm-ml.cpp llvm/tools/llvm-objdump/MachODump.cpp llvm/tools/llvm-objdump/llvm-objdump.cpp llvm/tools/llvm-profgen/ProfiledBinary.cpp llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp llvm/tools/sancov/sancov.cpp llvm/unittests/CodeGen/MachineInstrTest.cpp llvm/unittests/CodeGen/MachineOperandTest.cpp llvm/unittests/CodeGen/TestAsmPrinter.cpp llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp llvm/unittests/MC/DwarfLineTables.cpp llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp Index: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp === --- llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp +++ llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp @@ -57,6 +57,7 @@ std::unique_ptr MRI; std::unique_ptr MUPMAI; std::unique_ptr MII; + std::unique_ptr MSTI; std::unique_ptr Str; std::unique_ptr Parser; std::unique_ptr Ctx; @@ -86,6 +87,10 @@ MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCOptions)); EXPECT_NE(MAI, nullptr); +std::unique_ptr MSTI; +MSTI.reset(TheTarget->createMCSubtargetInfo(TripleName, "", "")); +EXPECT_NE(MSTI,
