Page MenuHomePhabricator

[IPRA] Interprocedural Register Allocation - Transformation Pass

Authored by vivekvpandya on Jun 9 2016, 6:32 AM.


This commit adds required transformation pass for IPRA and a simple testcase to verify it.

  1) lib/Target/X86/X86RegUsageInfoPropagate.cpp (Target Specific MachineFunction Pass) to update RegMasks of call instr in caller function based on the RegMask returned by immutable pass, scheduled at Pre-RegAlloc.

Diff Detail


Event Timeline

vivekvpandya retitled this revision from to [IPRA] Interprocedural Register Allocation - Transformation Pass.
vivekvpandya updated this object.

Two very basic questions:
1.) Do we plan on supporting IPRA on other targets? My assumption is yes (please).
2.) Can the RegUsageInfoPropagate pass be written in a target-independent way and add target specific hooks as necessary?

mehdi_amini edited edge metadata.Jun 9 2016, 9:06 AM

I don't know why the pass is still X86 specific, I thought our conclusion before splitting the patch was that it is not and should be a generic pass?

The only reason it is X86 was that we thought we would have to handle calls in a target specific way, but it turned out we should be able to get away with that.

vivekvpandya updated this object.
vivekvpandya edited edge metadata.

This commit makes IPRA transformation a target independent pass.

mehdi_amini added inline comments.Jun 9 2016, 10:41 PM
14 ↗(On Diff #60313)

trailing whitespace

mehdi_amini added inline comments.Jun 9 2016, 10:47 PM
109 ↗(On Diff #60313)

Take a Function * as argument here, and adapt call sites.

122 ↗(On Diff #60313)

no braces

11 ↗(On Diff #60313)

Add some comment in the test describing what you're checking in both cases.

Simple comment added in test case.

mcrosier added inline comments.Jun 10 2016, 6:51 AM
1 ↗(On Diff #60346)

This doesn't appear to be 80-column.

13 ↗(On Diff #60346)

MachineInstrs in a given

14 ↗(On Diff #60346)

RegisterUsageInfo.cpp -> RegisterUsageInfo

15 ↗(On Diff #60346)

of the callee

Perhaps something like:
if the RegMask information is available then this pass will update the RegMask of the call instruction.

17 ↗(On Diff #60346)

used by the register allocator while allocating the current MachineFunction.

42 ↗(On Diff #60346)

I've seen this style in the past:

#define RUIP_NAME "Register Usage Information Propagation"

and then replace the strings with the #define.

53 ↗(On Diff #60346)
return  RUIP_NAME;
75 ↗(On Diff #60346)

"Register Usage Information Propagation" -> RUIP_NAME

78 ↗(On Diff #60346)

"Register Usage Information Propagation" -> RUIP_NAME

102 ↗(On Diff #60346)

I'm not sure this comment is necessary, but I don't have any strong feelings for or against it. However, if you do decide to keep it, please write all comments using proper English prose (capitalization, puncuation, etc.).

103 ↗(On Diff #60346)

You can reduce the indent by changing this check to:

if (!MI.isCall())

I tend to think it also improves readability.

mcrosier added inline comments.Jun 10 2016, 7:01 AM
74 ↗(On Diff #60346)

The second argument to INITIALIZE_PASS_BEGIN/END is used to specify the name of the command-line option passed to llc/opt to enable this pass. Generally, this is all lower case with dashes are used for spacing (e.g., "peephole-opt", "machine-sink", "if-converter"). We should decide upon a name that uses this convention (e.g., "reg-usage-propagation").

changes as suggested by @mcrosier.

mehdi_amini added inline comments.Jun 10 2016, 9:06 AM
118 ↗(On Diff #60359)

No: Operand.getGlobal() already returns the Function *....
(you'll need a cast)

mehdi_amini added inline comments.Jun 10 2016, 9:54 AM
110 ↗(On Diff #60365)
if (!RegMask)
119 ↗(On Diff #60365)

No unsafe C-like cast, use the llvm style of casting please: cast<Function>

mehdi_amini accepted this revision.Jun 10 2016, 11:37 AM
mehdi_amini edited edge metadata.


This revision is now accepted and ready to land.Jun 10 2016, 11:37 AM
This revision was automatically updated to reflect the committed changes.