This is an archive of the discontinued LLVM Phabricator instance.

[IPRA] Interprocedural Register Allocation - Transformation Pass
ClosedPublic

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

Details

Summary
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
test/CodeGen/Generic/ipra-transform.ll
15

trailing whitespace

mehdi_amini added inline comments.Jun 9 2016, 10:47 PM
lib/CodeGen/RegUsageInfoPropagate.cpp
110

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

123

no braces

test/CodeGen/Generic/ipra-transform.ll
12

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
lib/CodeGen/RegUsageInfoPropagate.cpp
2

This doesn't appear to be 80-column.

14

MachineInstrs in a given

15

RegisterUsageInfo.cpp -> RegisterUsageInfo

16

of the callee

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

18

used by the register allocator while allocating the current MachineFunction.

43

I've seen this style in the past:

#define RUIP_NAME "Register Usage Information Propagation"

and then replace the strings with the #define.

54
return  RUIP_NAME;
76

"Register Usage Information Propagation" -> RUIP_NAME

79

"Register Usage Information Propagation" -> RUIP_NAME

103

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.).

http://llvm.org/docs/CodingStandards.html#commenting

104

You can reduce the indent by changing this check to:

if (!MI.isCall())
  continue;

I tend to think it also improves readability.

mcrosier added inline comments.Jun 10 2016, 7:01 AM
lib/CodeGen/RegUsageInfoPropagate.cpp
75

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
lib/CodeGen/RegUsageInfoPropagate.cpp
118

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

mehdi_amini added inline comments.Jun 10 2016, 9:54 AM
lib/CodeGen/RegUsageInfoPropagate.cpp
111
if (!RegMask)
  return;
120

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.

LGTM.

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.