Page MenuHomePhabricator

[MC] Untangle MCContext and MCObjectFileInfo
ClosedPublic

Authored by flip1995 on Apr 28 2021, 7:56 AM.

Details

Summary

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

Diff Detail

Event Timeline

flip1995 created this revision.Apr 28 2021, 7:56 AM
flip1995 requested review of this revision.Apr 28 2021, 7:56 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2021, 7:56 AM

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.

MaskRay added a comment.EditedApr 28 2021, 3:52 PM

The refactoring part seems useful on its own. The controversial 4-byte alignment part should be split.

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.

MaskRay added a comment.EditedApr 29 2021, 9:22 AM

The InitMCObjectFileInfo refactoring is good, but I'll suggest initMCObjectFileInfo(MCContext &ctx, bool PIC, bool LargeCodeModel) (change the function name to lowerCase, and reorder MCContext before bool arguments): if you are changing the signature and all the call sites, you can just fix the function naming.

The refactoring adding Triple to MCContext::MCContext and adding setObjectFileInfo should be separate. We sill want to see whether that is good.

The RISC-V functionality change should be separate as well.

shafik added a subscriber: shafik.Apr 29 2021, 1:52 PM
shafik added inline comments.
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
908

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!

shafik added inline comments.Apr 29 2021, 2:01 PM
clang/lib/Parse/ParseStmtAsm.cpp
590

/*PIC=*/false

llvm/tools/llvm-mca/llvm-mca.cpp
384

/*PIC=*/ false

see clang-tidy rule that checks for this format: https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

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 👍

flip1995 updated this revision to Diff 342345.May 3 2021, 3:32 AM

[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
flip1995 retitled this revision from Make it possible for targets to define their own MCObjectFileInfo to [MC] Untangle MCContext and MCObjectFileInfo.May 3 2021, 3:33 AM
flip1995 edited the summary of this revision. (Show Details)
flip1995 updated this revision to Diff 342352.May 3 2021, 4:08 AM
flip1995 marked 3 inline comments as done.
flip1995 edited the summary of this revision. (Show Details)

Rename InitMCObjectFileInfo and reorder arguments

Herald added a project: Restricted Project. · View Herald Transcript
flip1995 updated this revision to Diff 342354.May 3 2021, 4:10 AM

Fix arc mistake...

bcain added a subscriber: bcain.May 3 2021, 7:53 AM
MaskRay accepted this revision.May 3 2021, 9:47 AM

Looks great!

This revision is now accepted and ready to land.May 3 2021, 9:47 AM

Not sure how the process from here on out is. I think it is important to note, that I don't have push rights and someone else will have to land this for me (I guess?).

Not sure how the process from here on out is. I think it is important to note, that I don't have push rights and someone else will have to land this for me (I guess?).

I'll keep this open for a few days as it touches too many things.
I can push it on your behalf afterwards but you'll need to provide your name and email https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator
(If you uploaded the diff with with git format-patch -1 instead of git diff the information would have been available)

flip1995 added a comment.EditedMay 3 2021, 10:11 AM

I'll keep this open for a few days as it touches too many things.

Sounds good 👍

but you'll need to provide your name and email

I used arc diff. The commits I made with git have my name and email attached. But it seems like arc doesn't use them? I'll figure it out tomorrow, can't be that hard, I hope.

Ah you just need that to commit it. In that case: "Philipp Krones <philipp.krones@embecosm.com>"

Hi @flip1995
My apologies, but you might have to rebase this on the latest main. I introduced a MCSubtargetInfo field in https://reviews.llvm.org/D100975. Some of the existing changes to the file may not be needed anymore.

This revision was landed with ongoing or failed builds.May 5 2021, 10:03 AM
This revision was automatically updated to reflect the committed changes.

I'll keep this open for a few days as it touches too many things.

Sounds good 👍

but you'll need to provide your name and email

I used arc diff. The commits I made with git have my name and email attached. But it seems like arc doesn't use them? I'll figure it out tomorrow, can't be that hard, I hope.

Ah you just need that to commit it. In that case: "Philipp Krones <philipp.krones@embecosm.com>"

Pushed:) You are right. A diff applied with arc diff can be fetched locally with arc patch D101462 with the author information.
(Sometimes arc patch fail refuse to apply and then I'll have no idea how to get the author information.)

I'll keep this open for a few days as it touches too many things.

Sounds good 👍

but you'll need to provide your name and email

I used arc diff. The commits I made with git have my name and email attached. But it seems like arc doesn't use them? I'll figure it out tomorrow, can't be that hard, I hope.

Ah you just need that to commit it. In that case: "Philipp Krones <philipp.krones@embecosm.com>"

Pushed:) You are right. A diff applied with arc diff can be fetched locally with arc patch D101462 with the author information.
(Sometimes arc patch fail refuse to apply and then I'll have no idea how to get the author information.)

arc amend lets you replace the metadata with the same thing arc patch adds.