[PATCH] D18421: [WIP] MachineFunction Properties

[Derek Schuff via llvm-commits]

dschuff added inline comments.

================
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 {
----------------
qcolombet wrote:
> 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.
This kind of use does seem reasonable; that kind of thing would have the caveat that in order to use it, any pass or function that added load or store instructions would have to remember to set the property. I'll have to think a bit more about what the extension mechanism would look like. A lot of the simplicity of this current form is owed to the fact that all of the bits are specified in one place.

================
Comment at: include/llvm/CodeGen/MachineFunction.h:107
@@ +106,3 @@
+    kTracksLiveness,
+    kAllVRegsAllocated,
+    kLastProperty,
----------------
tstellarAMD wrote:
> You may want to clarify what kAllVRegsAllocated means, because targets are allowed to introduce new virtual registers after register allocation in TargetInstrInfo::eliminateFrameIndex().  These are eliminated by PEI using the register scavenger.
Yes; although notably this all happens within the run of the PEI pass. So while it has a transient state, it does preserve the invariant.
But yes, the exact meaning of these should be clarified, and we should probably revisit the naming of things.

================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:56
@@ +55,3 @@
+   return *RequiredProperties;
+ }
+ MachineFunctionProperties &getSetProperties() { return *SetProperties; }
----------------
qcolombet wrote:
> 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.
(addressed with a diferent interface style; see below)

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

================
Comment at: lib/CodeGen/FuncletLayout.cpp:29
@@ -28,1 +28,3 @@
+    getRequiredProperties().set(
+        MachineFunctionProperties::Property::kAllVRegsAllocated);
   }
----------------
qcolombet wrote:
> 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.
I originally did it this way to avoid having to call getRequiredProperties (and thus construct the required, set, and cleared properties on every function). However I found another way around that, so I change it to be more like getAnalysisUsage, which I agree is nicer (and now has the added benefit that e.g. the required properties can't accidentally get modified during runtime).

Because these properties only apply to MachineFunctionPasses, and because Pass managers only deal with FunctionPasses, I still think it's cleaner to do the checking in MachineFunctionPass::runOnFunction() instead of propagating all of this stuff out to the pass manager via public interfaces. Note that as it is right now, it still has the nice property that the dependencies are automatically checked for us (where "us" is the authors of machine function passes).

================
Comment at: lib/Target/NVPTX/NVPTXTargetMachine.cpp:229
@@ -226,1 +228,3 @@
+  disablePass(&PostRASchedulerID);
+  disablePass(&FuncletLayoutID);
 
----------------
qcolombet wrote:
> This is an unrelated change, right?
Actually, it's not; these are all passes that are running today in NVPTX and WebAssembly that were originally written as post-RA passes, but have no particular assertions or dependencies on the lack of virtual registers, and so were not explicitly disabled. (They mostly do nothing for virtual architectures). But since I've now added this requiredProperty mechanism, they now have to be disabled, or else they will fail the check. 

So basically this is exactly the effect that you wanted all along; these passes can't be run for virtual targets now (until and unless someone ensures that they support virtual registers and changes the requirement).


http://reviews.llvm.org/D18421