[PATCH] D20769: [IPRA] Interprocedural Register Allocation

[Vivek Pandya via llvm-commits]

vivekvpandya marked an inline comment as done.

================
Comment at: include/llvm/CodeGen/MachineOperand.h:146
@@ -147,1 +145,3 @@
+    unsigned RegNo;    // For MO_Register.
+    unsigned OffsetLo; // Matches Contents.OffsetedInfo.OffsetHi.
   } SmallContents;
----------------
mehdi_amini wrote:
> It is not good practice to clang-format the whole file as part of your patch. You should only format the patch of the file you changed (git clang-format handles it automatically, some text editor as well, or you can give a range on the command line, or even copy paster on stdin the range you want to format).
I will consider it, do we required to revert those changes ? If yes then please suggest easy way, I am using git.

================
Comment at: include/llvm/CodeGen/PhysicalRegisterUsageInfo.h:29
@@ +28,3 @@
+class StringRef;
+
+class PhysicalRegisterUsageInfo : public ImmutablePass {
----------------
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.

================
Comment at: lib/CodeGen/CMakeLists.txt:104
@@ -102,2 +103,3 @@
   RegisterScavenging.cpp
+  RegUsageInfoCollector.cpp
   SafeStack.cpp
----------------
qcolombet wrote:
> Looks like this patch could be split:
> - One patch for RegisterUsageInfo.
> - One patch for InfoCollector.
@qcolombet Could you please explain why is that required?

================
Comment at: lib/CodeGen/PhysicalRegisterUsageInfo.cpp:1-2
@@ +1,3 @@
+//=- PhysicalRegisterUsageInfo.cpp - Register Usage Informartion Storage -*- C++
+//-*-=//
+//
----------------
MatzeB wrote:
> strange linebreak. I think in .cpp we also don't need those magic markers for the emacs users (my understanding is that they are just used in the .h files so emacs knows they contain C++ code and not just C code).
oh yes this is already mentioned in coding standards but I really didn't check for it. 
Sorry.

================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:37
@@ +36,3 @@
+namespace llvm {
+void initializeRegUsageInfoCollectorPass(PassRegistry &);
+}
----------------
mehdi_amini wrote:
> Doesn't this need to be in a header and be called somewhere?
> It's not clear to me how is this pass registered?
Isn't this will be called by method generated due to macroINITIALIZE_PASS_BEGIN ?

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:514
@@ +513,3 @@
+namespace {
+class DummyCGSCCPass : public CallGraphSCCPass {
+public:
----------------
mehdi_amini wrote:
> Looks like we'll need a home for this pass, can't really leave it there. I'm not sure where to put it yet.
Have thought of any thing for this?

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:529
@@ +528,3 @@
+INITIALIZE_PASS_DEPENDENCY(CallGraphWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
+INITIALIZE_PASS_END(DummyCGSCCPass, "DummyCGSCCPass", "DummyCGSCCPass", false,
----------------
mehdi_amini wrote:
> Are these dependency required?
I believe CallGraphWrapperPass is required, though I have not look at this code closely as it was given by you :D 

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:529
@@ +528,3 @@
+INITIALIZE_PASS_DEPENDENCY(CallGraphWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
+INITIALIZE_PASS_END(DummyCGSCCPass, "DummyCGSCCPass", "DummyCGSCCPass", false,
----------------
vivekvpandya wrote:
> mehdi_amini wrote:
> > Are these dependency required?
> I believe CallGraphWrapperPass is required, though I have not look at this code closely as it was given by you :D 
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.

================
Comment at: lib/Target/X86/X86RegUsageInfoPropagate.cpp:101
@@ +100,3 @@
+
+  bool changed = false;
+
----------------
MatzeB wrote:
> We start local variables with uppercase letters (coding convention).
I thought it is not for primitive type. I will change it.  


http://reviews.llvm.org/D20769