[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Mehdi AMINI via llvm-commits]

mehdi_amini added a subscriber: mehdi_amini.
mehdi_amini added a comment.

Good start!

First a few high-level comments:

- generate your patch with full context (git diff -U9999, or use arc)
- Stick with the LLVM coding conventions during your development, it will be easier on the review (variables starts with upper case, use clang-format or `git clang-format` as much as possible).
- Use for-range loop when possible.

The major problem in your patch, and the source of the non-determinism is that you didn't track closely the ownership of the RegMask vector and you made a local copy in the `RegUsageInfoPropagationPass`. The operand is then updated with a pointer to this local copy that is destroyed when it goes out-of-scope.

See inline some comments.


================
Comment at: include/llvm/CodeGen/MachineOperand.h:537
@@ -536,1 +536,3 @@
 
+  void setRegMask(uint32_t *RegMaskPtr) {
+    assert(isRegMask() && "Wrong MachineOperand mutator");
----------------
Take a const ptr.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:36
@@ +35,3 @@
+			PassRegistry &Registry = *PassRegistry::getPassRegistry();
+    		initializePhysicalRegisterUsageInfoPass(Registry);
+		}
----------------
You need to do the same in RegUsageInfoPropagationPass

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:43
@@ +42,3 @@
+
+  	void storeUpdateRegUsageInfo(StringRef MFName, std::vector<uint32_t> &RegMasks);
+
----------------
Take RegMasks by value here.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:45
@@ +44,3 @@
+
+  	std::vector<uint32_t> getRegUsageInfo(StringRef MFName);
+
----------------
`const std::vector<uint32_t> *getRegUsageInfo(StringRef MFName);`

================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:32
@@ +31,3 @@
+															std::vector<uint32_t> &RegMask) {
+	RegMasks[MFName] = RegMask;
+}
----------------
`RegMasks[MFName] = std::move(RegMask);`

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:99
@@ +98,3 @@
+
+	PRUI->storeUpdateRegUsageInfo(MF.getName(),RegMask);
+
----------------
` PRUI->storeUpdateRegUsageInfo(MF.getName(), std::move(RegMask));`

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:527
@@ -492,2 +526,3 @@
 void TargetPassConfig::addISelPrepare() {
+  
   addPreISel();
----------------
Remove

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:587
@@ -545,2 +586,3 @@
 
+
   // Expand pseudo-instructions emitted by ISel.
----------------
remove

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:603
@@ -557,1 +602,3 @@
+    addPass(new PhysicalRegisterUsageInfo); 
+  }
 
----------------
Remove this, it is useless.

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:668
@@ -614,3 +667,3 @@
   addPreEmitPass();
-
+ 
   addPass(&FuncletLayoutID, false);
----------------
Remove

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:56
@@ +55,3 @@
+		private:
+			static void setRegMask(MachineInstr &MI, uint32_t *RegMask) {
+				for (MachineOperand &MO : MI.operands()) {
----------------
Take a const ptr

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:92
@@ +91,3 @@
+		for(MachineBasicBlock::iterator MIB = mb->begin(), MIE = mb->end(); MIB != MIE; MIB++) {
+			MachineInstr *mi = MIB;
+			//is it a call instr ?
----------------
C++11 for-range:

```
  for(auto &MBB : MF) {
    for(auto &MI : MBB) {
```

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:113
@@ +112,3 @@
+					}	
+				}
+				
----------------
```
        auto updateRegMask = [&](StringRef FuncName) {
          const auto *RegMask = PRUI->getRegUsageInfo(FuncName);
          if (RegMask) { // else skip optimization
            setRegMask(MI, &(*RegMask)[0]);
            changed = true;
          }
        };
        MachineOperand &Operand = MI.getOperand(0);
        if (Operand.isGlobal()) {
          updateRegMask(Operand.getGlobal()->getName());
        } else if (Operand.isSymbol()) {
          updateRegMask(Operand.getGlobal()->getName());
        }
```

================
Comment at: lib/Target/X86/X86TargetMachine.cpp:316
@@ -313,1 +315,3 @@
+  if(UseIPRA) 
+    addPass(createRegUsageInfoPropPass());
 }
----------------
This is not the right place, it should be added right after `createX86ISelDag`


http://reviews.llvm.org/D20769