[PATCH] D18421: [WIP] MachineFunction Properties

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Derek,

I like the direction of your patch. I would go one step further and have the pass manager checks that required properties are set before running a pass (and error out if this is false).

I could see value in having the properties extendable by the target, in particular if the pass manager checks the dependencies for us.

Couple of comments inlined.

Thanks,
-Quentin


================
Comment at: include/llvm/CodeGen/MachineFunction.h:95
@@ +94,3 @@
+// Each of these has checking code in the MachineVerifier, and passes can
+// require that a property be set.
+class MachineFunctionProperties {
----------------
I think it would be nice to have this extendable by the target, especially if the properties are checked and/or give some control over the compilation pipeline.
E.g., On ARM, passes could set a property has load/store and the load/store optimizer would be only run if such property is true. The example is a bit lame, but you get the idea.

Obviously, this can already be achieved by querying within the passes the MachineFunctionInfo instances, but it sounds reasonable to have this kind of broad mechanism.

================
Comment at: include/llvm/CodeGen/MachineFunction.h:108
@@ +107,3 @@
+    kAllVRegsAllocated,
+    kLastProperty,
+  };
----------------
I think this does not follow the coding standard:
Enumerators (e.g. enum { Foo, Bar }) [...] should start with an upper-case letter, just like types.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:56
@@ +55,3 @@
+   return *RequiredProperties;
+ }
+ MachineFunctionProperties &getSetProperties() { return *SetProperties; }
----------------
We may allow to return nullptr. Not all passes have properties.
Another other would be to have a static const empty set MachineFunctionProperties, used when those pointers are null.

================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:65
@@ +64,3 @@
+ MachineFunctionProperties *SetProperties = nullptr;
+ MachineFunctionProperties *ClearedProperties = nullptr;
+
----------------
If you go that way, I would make them lazily allocated.
I.e., mutable should be in order :).

================
Comment at: lib/CodeGen/FuncletLayout.cpp:29
@@ -28,1 +28,3 @@
+    getRequiredProperties().set(
+        MachineFunctionProperties::Property::kAllVRegsAllocated);
   }
----------------
My 2c; I would find it more natural to define this kind of dependencies in something like getAnalysisUsage, possibly something new.

The value, I see out of this, is the pass manager could check this kind of dependencies for us and error out is something is not correct.

================
Comment at: lib/Target/NVPTX/NVPTXTargetMachine.cpp:229
@@ -226,1 +228,3 @@
+  disablePass(&PostRASchedulerID);
+  disablePass(&FuncletLayoutID);
 
----------------
This is an unrelated change, right?

================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:110
@@ -109,1 +109,3 @@
+  void addMachineLateOptimization() override;
+  bool addGCPasses() override { return false; }
   void addPreEmitPass() override;
----------------
Ditto.

================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:186
@@ -182,1 +185,3 @@
+  // Has no asserts of its own, but was not written to handle virtual regs.
+  disablePass(&ShrinkWrapID);
   // We use our own PrologEpilogInserter which is very slightly modified to
----------------
Ditto.

================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:214
@@ -204,1 +213,3 @@
+}
+
 void WebAssemblyPassConfig::addPreEmitPass() {
----------------
Ditto.

================
Comment at: lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:222
@@ -209,1 +221,3 @@
+  disablePass(&StackMapLivenessID);
+  disablePass(&LiveDebugValuesID);
 
----------------
Ditto.


http://reviews.llvm.org/D18421