[IPRA] Interprocedural Register Allocation - Analysis Passes

[vivekvpandya]

Summary

 This commit addes pass to change CodeGen order to be bottom up order on Call Graph. The patch for the same has been provided by dear Mehdi Amini.
 
	This commit added required analysis passes for IPRA.
 
 1) lib/CodeGen/RegUsageInfoCollector.cpp (MachineFunction Pass) to create RegMask based on actual register usage, scheduled at POST-RA
 2) lib/CodeGen/RegisterUsageInfo.cpp (Immutable Pass) to store RegMask details for functions which has been processed in bottom up order on Call Graph, Scheduled before RegAllocation.
   
 This commit also adds a method setRegMask() to MachineOperand class.
 This commit also adds command line option to enable IPRA -enable-ipra and debug type "ip-regalloc".
 Changes to adher to coding standards, correction for ownership of RegMask vector in related functions.

[hfinkel]

include/llvm/CodeGen/PhysicalRegisterUsageInfo.h

10–11 ↗

(On Diff #59113)

How about:

// This pass is required to take advantage of the interprocedural-register-allocation infrastructure.

15 ↗

(On Diff #59113)

If you change from using a StringMap, update this comment.

54 ↗

(On Diff #59113)

I'd also prefer a GlobalVariable * here, as that currently establishes our canonical function identify. If we work to further separate the MI level from the IR level, I suspect we'd establish some other function object which would have pointer-based identify, and we can always then update this code to use that instead.

[mehdi_amini]

include/llvm/CodeGen/PhysicalRegisterUsageInfo.h

54 ↗

(On Diff #59113)

Since MatzeB commented about the allocation in std::vector in the MachineFunctionPass, it seems equally important to override doInitialization(Module &M) in order to reserve the size of the map with the number of functions defined in the module, and doFinalization to clear the map.
(clearing the map is actually a correctness issue)

lib/CodeGen/RegUsageInfoCollector.cpp

89

What is regMaskSize on ARM and X86?
Before moving on with stack allocation here, I think we have to consider that:

• First this is ran once per function, so having one malloc per function during codegen does not make it an expensive analysis.
• Second the vector will be moved in the immutable pass map. So having a SmallVector makes it less inefficient to move and store (we may not want to have a DenseMap anymore, and thus we'd have to make an extra malloc there!).

[vivekvpandya]

include/llvm/CodeGen/MachineOperand.h

147

I will consider it, do we required to revert those changes ? If yes then please suggest easy way, I am using git.

include/llvm/CodeGen/PhysicalRegisterUsageInfo.h

29 ↗

(On Diff #58934)

Actually I run git clang-format , but that was after committing my changes so it did not changed any thing. I have applied clang-format on each file.

lib/CodeGen/CMakeLists.txt

103

@qcolombet Could you please explain why is that required?

lib/CodeGen/PhysicalRegisterUsageInfo.cpp

1–2 ↗

(On Diff #59113)

oh yes this is already mentioned in coding standards but I really didn't check for it.
Sorry.

lib/CodeGen/RegUsageInfoCollector.cpp

38

Isn't this will be called by method generated due to macroINITIALIZE_PASS_BEGIN ?

lib/CodeGen/TargetPassConfig.cpp

502

Have thought of any thing for this?

517

I believe CallGraphWrapperPass is required, though I have not look at this code closely as it was given by you :D

517

No these dependencies are not required, I am yet to find why not technically but I just removed it and compiled llvm again and things are working.

lib/Target/X86/X86RegUsageInfoPropagate.cpp

101 ↗

(On Diff #59113)

I thought it is not for primitive type. I will change it.

[mehdi_amini]

include/llvm/CodeGen/PhysicalRegisterUsageInfo.h

30 ↗

(On Diff #59113)

git clang-format takes a commit range, so you can format patches that are already committed.
(i.e, git clang-format HEAD~ formats the last commit, you can just amend it afterward)

lib/CodeGen/RegUsageInfoCollector.cpp

38

If there is something in the macro expansion that makes you think it is called, please elaborate.

[vivekvpandya]

lib/CodeGen/RegUsageInfoCollector.cpp

38

No, I look carefully at that macro it provides definition for initialize.. method but yes some where we need to call that method. Also I haven't mentioned this pass as dependency for any other pass with INITIALIZE_PASS_DEPENDENCY other wise it can call that function. Now I am also having same question as you.

[vivekvpandya]

lib/Target/X86/X86TargetMachine.cpp

283 ↗

(On Diff #59113)

@hfinkel X86RegUsageInfoPropagate.cpp will be generic pass soon then I will do this change too.

[vivekvpandya]

Correction in typos , more adhering to coding standards.
PhysicalRegisterUsageInfo.cpp -> RegisterUsageInfo.cpp
DenseMap of GlobelVariable * to RegMask is used instead of StringMap.
DummyCGSCCPass moved to CallGraphSCCPass.h and .cpp

[mehdi_amini]

We're getting there, the patch is looking really good now! :)

Still a little bit of work ahead, see inline comments.

include/llvm/Analysis/CallGraphSCCPass.h

115–130

Describe the class with a doxygen comment.

include/llvm/CodeGen/MachineOperand.h

537

Do not repeat the name of the function in the doxygen comment (other places that do it are outdated).

include/llvm/CodeGen/Passes.h

366

s/used/preserved/

include/llvm/CodeGen/RegisterUsageInfo.h

14

You shouldn't introduce implementation details in high-level description.

lib/CodeGen/CMakeLists.txt

103–105

It is a good practice to decoupled software component. Having separate patches helps to make sure we indeed have correctly separated the components.
It also forces to make sure the component are individually testable, and make sure we actually test them.

So I agree with Quentin on the principle, and I think it is also a good exercise for you to split the patch an submit the analysis alone and tested. Have a look at "CostModelAnalysis::print()" in lib/Analysis/CostModel.cpp and see how it is tested in test/Analysis/CostModel/X86/cast.ll

lib/CodeGen/RegUsageInfoCollector.cpp

12

Wrapping

lib/CodeGen/TargetPassConfig.cpp

119

Why "compile time"? I'd just write "Enable inter-procedural register allocation"

516

You forgot to remove this hunk

[vivekvpandya]

lib/CodeGen/CMakeLists.txt

103–105

@mehdi_amini I understand what you explain above but here I think RegisterUsageInfo is not tastable alone because it just holds RegMasks, RegisterInfoCollector is a trigger to IP regalloc and both of them can be tested together. Also to test X86RegUsageInfoPropagate it requires both of the above mentioned passes. But we can separate patches for RegisterUsageInfo + InfoCollector and X86RegUsageInfoPropagate ( condition that first patch is required to test second one) .
Is there any better plan in your mind?

[mehdi_amini]

lib/CodeGen/CMakeLists.txt

103–105

Splitting in two is what I had in mind: the analysis part on one side, the transformation part on another.

[vivekvpandya]

lib/CodeGen/CMakeLists.txt

103–105

Just to make sure RegisterUsageInfo.cpp and RegUsageInfoCollector.cpp both are part of analysis so there is not need of separate patch for them but I will separate changes related to X86RegUsageInfoPropagate.cpp in other patch.

[vivekvpandya]

@mehdi_amini I have made all suggested changes , I am writing a simple test for RegUsageInfoCollector so that next review you can also look at that. Thanks for your patience.

[vivekvpandya]

patch is splited into analysis and optimization pass. Simple test cases has been added.

[mehdi_amini]

include/llvm/CodeGen/RegisterUsageInfo.h

50

add a method print() like other analysis, and call it here behind a flag.

55

Doxygen

58

Doxygen

test/CodeGen/Generic/reg-usage-info.ll

3

I don't like that we can't pass the test in release mode. I suggested that the dump occurs in doFinalization() for the analysis pass (i.e. PhysicalRegisterUsageInfo). You need to implement a proper cl::opt that controls a dump there, that could be used in release mode.

5

I'm worried that we don't provide any stability guarantee on the numbers printed here.
Having a nicer textual form would be better.
I just don't see how to do it other than keep a pointer to the TRI in the DenseMap in the immutable pass to be able to get the register name.

[vivekvpandya]

test/CodeGen/Generic/reg-usage-info.ll

3

Aren't we refering

RegUsageInfoCollector

as analysis pass,

RegisterUsageInfo

is there for keeping data around.

5

If RegUsageInfoCollector is considered as analysis pass then we can add cl::opt into that file and have analysis printed from that.

[mehdi_amini]

test/CodeGen/Generic/reg-usage-info.ll

3

Technically in LLVM the analysis is the pass that you require and query the result from (i.e. a pass you're using as getAnalysis()). But yeah terminology can be fuzzy.

[vivekvpandya]

test/CodeGen/Generic/reg-usage-info.ll

3

SO What is your suggestion? Which file should be changed?

[mehdi_amini]

test/CodeGen/Generic/reg-usage-info.ll

3

I think I already told you: the way analysis are tested at the IR level is using the -analyze flag in opt. Since we don't have -analyze in llc, the closest I can imagine is to implement the analysis (in the LLVM sense) with the "print(...)" method as other analyses, and call it in doFinalization() when a cl::opt flag is set.

[vivekvpandya]

test/CodeGen/Generic/reg-usage-info.ll

5

Will TRI pointer assigned a value by the MachineFunction pass ? Or Is there any way to get TargetMachine or TargetRegisterInfo from Module object? Also if PReg name is required to be printed then MCRegisterInfo will be required.

[vivekvpandya]

a new command line option dump-regusage has been added so that reg usage information can be printed in release build. The test case is also changed accordingly.

[mehdi_amini]

include/llvm/CodeGen/RegisterUsageInfo.h

52

Is the TRI dependent on the Target or the SubTarget?
If it is the latter, it can change on each function and thus needs to be in the map (at which point we better have a dedicated class for the map value)

57

Same question.

lib/CodeGen/RegisterUsageInfo.cpp

25

I'm not sure the name of the option is great, but I'll leave the naming to Quentin/Matthias.

50

Can you implement the print in a separate print method with the same signature as the other analyses for consistency?
And call it from here being the DumpRegUsage flag?

[mehdi_amini]

lib/CodeGen/RegUsageInfoCollector.cpp

129

Mdl is not usual, M alone is more common (or sometimes Mod)

[vivekvpandya]

lib/CodeGen/RegUsageInfoCollector.cpp

129

Ok I will take care for naming , but that would be not required if we are going with Function * , because MF.getFunction() would be enough.

[mehdi_amini]

lib/CodeGen/RegUsageInfoCollector.cpp

131

This is not the right API: getNamedGlobal is for global *variables*, not function.

lib/CodeGen/RegisterUsageInfo.cpp

57

Add an assertion that MFGlobalVar is not null (and the name isn't very explicit)

[vivekvpandya]

lib/CodeGen/RegisterUsageInfo.cpp

57

now I think such assertion wold be for Function *, right ?

[vivekvpandya]

lib/CodeGen/RegUsageInfoCollector.cpp

131

I tried out

PRUI->storeUpdateRegUsageInfo(Mdl->getGlobalVariable(MF.getFunction()->getName(),true), std::move(RegMask));

and

PRUI->storeUpdateRegUsageInfo(Mdl->getGlobalVariable(MF.getFunction()->getName()), std::move(RegMask));

but both returns nullptr for some functions.

[mehdi_amini]

lib/CodeGen/RegUsageInfoCollector.cpp

131

My previous comment called out the fact that you were using an API for global *variables* instead of function, and you replaced with a call to getGlobalVariable?

There is a getFunction() API on the module that returns only functions, or there is a getNamedValue() that will return any symbol.

However you have MF.getFunction(), so you should not call any of these.

[vivekvpandya]

RegMasks uses Function * as key, print method and TargetMachine * have added to RegisterUsageInfo. Command line option -dump-regusage has been renamed to -print-regusage, test case is also updated to reflect the changes.

[mehdi_amini]

lib/CodeGen/RegisterUsageInfo.cpp

65

pair is not coding-convention friendly.

Also you are iterating on a map that is keyed on pointer values, which does not provided any ordering guarantee. Even with names as keys, the map is unordered anyway.
You need to generate a vector and sort it first.

test/CodeGen/Generic/reg-usage-info.ll

39

This test is not OK, for the reasons I explained when I provided the other test (stability).

[vivekvpandya]

Test case changed to simple test case which is originally provided by Mehdi Amini, print() now sorts analysis details before printing in RegisterUsageInfo.cpp

[mehdi_amini]

lib/CodeGen/RegUsageInfoCollector.cpp

93

I think MatzeB mentioned that TRI is a subclass of MCRI, so I'm not sure why you're using MCRI at all while you have TRI.

124

Why aren't you using MachineOperand::clobbersPhysReg here?

test/CodeGen/Generic/reg-usage-info.ll

9

What will be printed for bar1 and bar2?
Add the CHECK lines here.

[vivekvpandya]

git clang-formated , test case changes, MCRI is not required , clobberedReg expression removed.

[mehdi_amini]

include/llvm/CodeGen/RegisterUsageInfo.h

59–60

Doxygen the two public API above.

lib/CodeGen/RegUsageInfoCollector.cpp

80

No braces

lib/CodeGen/RegisterUsageInfo.cpp

73

No braces.

lib/CodeGen/TargetPassConfig.cpp

505

No braces

633

No braces

[mehdi_amini]

test/CodeGen/Generic/reg-usage-info.ll

16

trailing whitespace here.

[qcolombet]

Hi Vivek,

I was thinking about debug ability and usability of the produced code.
Something to consider in a following patch would be to add in the assembly output comments at the beginning of the function and/or related call site of the function listing what are the register preserved by the function.

The rationale is that now if we play with assembly files produced by different optimization phase, we want a quick way to check if two functions are equivalent.
E.g.,
Let say we have two assembly files produced for the same application:

• bad.s which fails, produced with ipra
• good.s which works, produced without ipra

When bisecting which function causes the failure, it is important to know if it is fine to take a function foo from good.s and replace it into bad.s. This may not be possible if the preserve register are not compatible.

Cheers,
-Quentin

[vivekvpandya]

@qcolombet. It will be very help full, I will look into that. Do we need to keep it default or a separate switch to get those comments ?

[mehdi_amini]

LGTM, with one inline comment.

include/llvm/CodeGen/RegisterUsageInfo.h

60

Strip the "this method is provided to" everywhere.
And here add that it will return null if the function is not known.

[vivekvpandya]

inline-asm related test case added Thanks to Mehdi Amini, and previous test case modified so that it do not fail due to transformation pass for IPRA.

[mehdi_amini]

Do you have commit access?

[vivekvpandya]

No I do not have commit access.