D73307 directly computes the hash so this is no longer necessary.

Took one pass over the code.

• This needs more tests like with exception blocks kept in separate clusters (check if they are grouped)

Clang options for Basic Block Sections enabled in D68063 and D73674

In D73307#1942805, @MaskRay wrote:

In D73307#1932131, @rnk wrote:

At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen.

I second this suggestion. clang/lib/CodeGen/BackendUtil.cpp EmitAssemblyHelper::EmitAssemblyWithNewPassManager ModulePassManager may be a more appropriate place for this feature. There are examples from both sides:

In D68065#1940343, @MaskRay wrote:

Hi Sriraman,

Sounds like there is strong support for your patch, so let’s move forward on it.

I do have a few more code review items I’d like addressed if we can before we commit.
There are several nits about excess parentheses which do not fit with the rest of lld code.
Please scan the full patch and delete them. Rebase and upload a new diff so that I can patch my local repository with arc patch D68065.
It does not apply so it isn’t convenient for me to review the tests now.

In addition, we probably should move test/ELF/bb-sections-* tests to test/ELF/propeller/

Address recent reviewer comments.

Changes to description of flag, remove redundant check.

In D68065#1938447, @MaskRay wrote:

In D68065#1937331, @tmsriram wrote:

In D68065#1934866, @MaskRay wrote:

I am very glad to see that we have made progress by landing D68063 (llvm/CodeGen/CommandFlags.inc) and D73674 (llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp). Basic block sections is agreed to be useful even outside Properller.

There are several optimizations goals:

• Alignment inserting
• Automatic cache prefetching
• Large code model addressing can lower performance quite a bit. A post-link scheme can relax large code model addressting to small code model addressing.
• ...

Ping.

In D68065#1934866, @MaskRay wrote:

I am very glad to see that we have made progress by landing D68063 (llvm/CodeGen/CommandFlags.inc) and D73674 (llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp). Basic block sections is agreed to be useful even outside Properller.

There are several optimizations goals:

• Alignment inserting
• Automatic cache prefetching
• Large code model addressing can lower performance quite a bit. A post-link scheme can relax large code model addressting to small code model addressing.
• ...
  
  There is a CPU erratum that we want to mitigate.
• Intel's Jump Condition Code Erratum
  
  By making this change, we will go the object file level route: annotate object files with metadata so that certain transformations can be performed.
  
  Whether this scheme can satisfy the goals and avoid the erratum, and the uncertainty about how many stuff we will have to reinvent is my biggest concerns.
  
  On https://lists.llvm.org/pipermail/llvm-dev/2020-February/139543.html (my brainstorming), I mentioned we may achieve our goals and make it suitable for future optimizations by using a file format with more semantics (rather than an object file). I hope we can think more on this, rather than rush to conclusions "this is redoing full LTO. it can't scale"
  
  Considering the above points, I re-iterate my "Request Changes". We need a plan to prove that we can achieve our optimization goals while avoiding caveats (erratum).

Address @rnk comments:

@echristo Eric, is this alright now? Thanks.

Address reviewer comments.

Address reviewer comments. Fix test and delete blank line.

Address Reviewer comments:

Address Reviewer comments.

In D68065#1927117, @echristo wrote:

In general I think a lot of this looks pretty good. It could use some more comments - in particular to call out what a lot of this code is used for. It's not used on a typical linking path and so could be confusing to people going through the code.

-eric

Fix the comment for the option basicblock-sections.

In D68063#1918478, @MaskRay wrote:

Please still consider my suggestion that removes unrelated paragraphs from the summary :)

@eli.friedman @echristo Is this patch alright? I have added detailed comments as suggested by Eric to BBSectionsPrepare.cpp in https://reviews.llvm.org/D73674#change-lCLJRdamtrcE Thanks!

• Change to use MemoryBuffer rather than string for bbsections file
• Detailed comments in BBSectionsPrepare.cpp on basic block sections

Address Reviewer comments:

In D68063#1913667, @MaskRay wrote:

To be clear I think this is close to being acceptable

Agreed. I am still waiting for the resolution to my (very old) comment about:

In D68063#1913593, @efriedma wrote:

In LLVM core libraries, we generally want to accommodate use-cases that don't involve writing files to disk. This makes it easier to write tools targeting new use-cases. Here, for example, someone might want to try writing a JIT using a Propeller workflow. If SampleProfileLoader isn't supporting that, it should probably be fixed.

Mechanically, this seems fine... but it's also okay if we end up with a bit of similar code in multiple places. llc option parsing vs. lld option parsing is two distinct operations.

In D68063#1913547, @echristo wrote:

Also the commit message is awesome, but would be good to get the commit message represented as comments in lots of the final code if possible :)

In D68063#1913544, @echristo wrote:

Couple of inline comments. I'm trying pretty hard to be able to get rid of TargetOptions.h some day if possible. Any thoughts on ways to do this without?

Address Reviewer comments.

In D68063#1908455, @efriedma wrote:

I'm not completely comfortable passing the BB-sections list to the backend as a file path; generally we try to allow the "frontend" (clang/llc/etc.) to control all file I/O. But I'm not sure what the alternative looks like. I guess we could pass it as MemoryBuffer?

Otherwise looks fine.

@efriedma : Hi Eli, this is the parent patch of D73674. Appreciate if you could take a look at this too, thanks!

Rebase, address reviewer comments.

Rebase.

Add support for serializing Basic Block Section Type.

The number of relocations per input section with basic blocks is a few, eyeballing it is about 3-4 relocations including the extra jump relocation that gets added.

In D68065#1894573, @MaskRay wrote:

A Phabricator tip: marking every review comment Not applicable. is not needed. You can just click "Close" (status: Unsubmitted) but don't click Submit. When you upload a new Diff (with arc diff), the comments will be closed automatically.

Fix symbol shrinking code to incorporate reviewer comments.

In D73674#1877420, @tmsriram wrote:

In D73674#1877341, @efriedma wrote:

In D73674#1877267, @tmsriram wrote:

In D73674#1868439, @efriedma wrote:

I'm afraid the way this patch is written, we're going to have to substantially rewrite it to port to other targets. In particular, we're assigning blocks to sections in the asmprinter, which is way too late for targets that do branch relaxation as an MIR pass.

Hi Eli, What do you suggest we do here? An MIR pass that assigns sections to basic blocks?

Yes, that would make sense. Not completely sure where it should run, but we can adjust that later. Maybe we want it to happen before MachineBlockPlacement, so we can do section-aware block placement?

Create a new pass and assign sections to Machine Basic Blocks in this pass.

Address reviewer comments, new test for PC32 reloc.

Deleted changes to CodeGenPrepare.cpp, Function.*.

In D68065#1890650, @MaskRay wrote:

40+ comments have not been addressed.

In D73674#1877341, @efriedma wrote:

In D73674#1877267, @tmsriram wrote:

In D73674#1868439, @efriedma wrote:

I'm afraid the way this patch is written, we're going to have to substantially rewrite it to port to other targets. In particular, we're assigning blocks to sections in the asmprinter, which is way too late for targets that do branch relaxation as an MIR pass.

Hi Eli, What do you suggest we do here? An MIR pass that assigns sections to basic blocks?

Yes, that would make sense. Not completely sure where it should run, but we can adjust that later. Maybe we want it to happen before MachineBlockPlacement, so we can do section-aware block placement?

Is there some reason to emit "cold" and "exception" blocks into separate sections? Not sure what benefit you get from separating them.

Sections containing hot blocks can be grouped together, function splitting of cold basic blocks and placing all the cold blocks together and away from the hot blocks gives us better iTLB utilization.

I meant, is there some reason to emit "cold" and "exception" sections separately from each other. I understand the benefit of separating both of them from the hot blocks.

In D73674#1868439, @efriedma wrote:

I'm afraid the way this patch is written, we're going to have to substantially rewrite it to port to other targets. In particular, we're assigning blocks to sections in the asmprinter, which is way too late for targets that do branch relaxation as an MIR pass.

Add tests for more jmp opcodes other nits.

Address reviewer comments.

Add a test for ICF with bb sections.

Address reviewer comments, camel case, delete and rename methods.

Modify handling of NOP Filler.

Rename JumpRelocations to JumpInstrMods. As per Rui's suggestion, relocations have a specific meaning and these c modifiers to jump instructions should not be confused with actual relocations.

Remove reference to "propeller" from this patch. This is LLD support for basic block sections.

I think the patch series should probably be structured this way:

1. LLVM CodeGen: enables basic block sections.
2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
3. LLVM CodeGen: which enables the rest of Propeller options.
4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4
   
   Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.

Remove usage of "propeller". Fix header inclusion.

In D68049#1870094, @tmsriram wrote:

In D68049#1868623, @MaskRay wrote:

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

Remove usage of "propeller" from this patch, this is only about support for basic block sections.

In D68049#1870094, @tmsriram wrote:

In D68049#1868623, @MaskRay wrote:

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

In D68049#1868623, @MaskRay wrote:

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

Use MemoryBuffer::getFile instead of fstream.

Removed getBBSectionsList (moved to LLVM) and address other reviewer comments.

In D68049#1865967, @MaskRay wrote:

If you don't mind, I can push a Diff to this Differential which will address these review comments.

Address reviewer comments.

Rebase and rename PropellerProf to BBSectionsProf.

Rebase and make some suggested fixes.

Address reviewer comments. Delete changes to BasicBlock.h.