[PATCH] D41373: [GISel][RFC]: GlobalISel Combiner prototype

Tue Dec 19 16:39:28 PST 2017

aditya_nandakumar added a comment.

Thanks Quentin. Will update the patch with the comments shortly.
Regarding tracking progress, I don't have a really good solution - I suspect it's possible to detect loops when every transformation is written in tablegen, but with mixing in C++, a naive approach would be to issue some diagnostic/warning after some large number of iterations in GICombiner.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombiner.h:12
+// setup a CombinerInfo and call combineMachineFunction.
+//
+//===----------------------------------------------------------------------===//
----------------
qcolombet wrote:
> Use doxygen style comments
Thanks. Will do.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombiner.h:24
+#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
+
----------------
qcolombet wrote:
> All the includes but MachineIRBuilder could be removed.
> Use forward declarations for the type definitions.
Makes sense. Will update.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombiner.h:31
+public:
+  GICombiner(GICombinerInfo &CombinerInfo, TargetPassConfig *TPC);
+
----------------
qcolombet wrote:
> Why is the combiner info not const here, ditto for TPC?
> 
> Basically, I am guessing there are good reasons (I didn't look at the implementation yet), but the comments should make it clear from the API.
> 
> Also, is it valid to give nullptr for TPC?
> If not, we should use a reference as well.
I was wondering if the combiner info would have some state stored (possibly), and wanted only lvalue references (so the CombinerPass would create the Info on stack/heap and pass it in instead of passing in references to temporaries). Alternatively, the info could be copied (or passed in via unique_ptr. 
The TPC is not currently used - making it const is definitely correct. Not really sure about if it can be null to make it reference.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:1
+//== llvm/CodeGen/GlobalISel/GICombinerHelper.h ---------------- -*- C++ -*-==//
+//
----------------
qcolombet wrote:
> Ditto for the GI prefix.
Will Update.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:29
+public:
+  GICombinerHelper(MachineIRBuilder &B, MachineRegisterInfo &MRI)
+      : Builder(B), MRI(MRI) {
----------------
qcolombet wrote:
> The builder comes with MRI already (getMF().getRegInfo()).
> Should we still have it being an additional argument.
Good point. We can remove the MRI.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:34
+
+  bool tryCombineCopy(MachineInstr &MI);
+
----------------
qcolombet wrote:
> Is MI supposed to be a COPY or does this combine will do the check?
> 
> Basically, what is the precondition here.
Yes - in the implementation, it does an early return if it's not a copy. You're right in that we need to document the expected behavior.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:36
+
+  bool tryCombine(MachineInstr &MI);
+};
----------------
qcolombet wrote:
> We would need to document the API. I.e., what does the boolean mean? In particular, what happens to MI is a change has been made?
the boolean return value is currently only used by the Combiner to track whether any change has been made so it needs to iterate again. It's also used to indicate if the pass made any changes.
Also, returning true (like the legalizer/selector) assumes that the instruction has been taken care of, and it's not guaranteed that the instruction is still alive.
Yes - documentation here would make it clear. Will do that.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerInfo.h:28
+  GICombinerInfo(bool AllowIllegalOps, LegalizerInfo *LInfo)
+      : IllegalOpsAllowed(AllowIllegalOps), LInfo(LInfo) {}
+  virtual ~GICombinerInfo() = default;
----------------
qcolombet wrote:
> I'm guessing we would need an assert AllowIllegalOps || LInfo.
> 
> Also to answer one of the question, I believe we would want two different boolean:
> - Allow creating illegal ops or not
> - Legalize illegal ops or not
Yup - the assert makes sense here.Will update.

The two different boolean approaches also make sense.

================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerInfo.h:33
+  virtual bool combine(MachineInstr &MI, MachineIRBuilder &B,
+                       MachineRegisterInfo &MRI) const = 0;
+};
----------------
qcolombet wrote:
> Where should the legality checks be done?
> 
> Basically, we need a method that all the all the subclasses of GICombinerInfo can call for this.
> 
> Maybe something around those lines:
> combine(...) {
> combineImpl(...) // the thing that gets override
> /// check legality stuff
> }
That sounds like a good approach.
Alternatively/Additionally, we can document that, the users should call some API to replace instructions (say CombineTo(FromMI, ToMI)) - where we can check before the original instruction is deleted, we can check the legality of the new instruction we're creating before replacing them. Of course, theres no way to force this upon the targets. 

================
Comment at: include/llvm/CodeGen/GlobalISel/GMIPatternMatch.h:6
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+#ifndef LLVM_GMIR_PATTERNMATCH_H
----------------
qcolombet wrote:
> The header looks weird
Will fix that.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:262
+    return buildSub(Res, (getRegFromArg(UseArgs))...);
+  }
   MachineInstrBuilder buildSub(unsigned Res, unsigned Op0,
----------------
qcolombet wrote:
> Unrelated changes?
Might make sense to split this in a separate patch.
These build methods are used in the unit test for the patternMatchers.

================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:35
+
+class PreLegalizeCombiner : public MachineFunctionPass {
+public:
----------------
qcolombet wrote:
> The relationship between PreLegalizeCombiner and GICombiner looks strange to me.
> In particular, before looking at the actual implementation of runOnMachineFunction, there is nothing showing that this pass uses GICombiner.
> 
> I would have expected something like GICombiner is a pass and PreLegalizeCombiner inherit from it and only set its custom PreLegalizeCombinerInfo thing.
I went with the "has a" relationship with the GICombiner. Pretty much what the GICombiner does right now is the driving of the combines which felt like it belonged in a utility. Right now, there should be little/no difference to switch to each pass inheriting the GICombiner and I can do that if required.

================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:41
+
+  bool combineMachineFunction(MachineFunction &MF);
+  StringRef getPassName() const override { return "PreLegalizeCombiner"; }
----------------
qcolombet wrote:
> Unless I missed it, there is no implementation for that.
This doesn't belong here (it's in the GICombiner). Good catch. Will remove that.

================
Comment at: lib/CodeGen/GlobalISel/CMakeLists.txt:6
+        GICombinerHelper.cpp
+        PreLegalizeCombiner.cpp
         IRTranslator.cpp
----------------
qcolombet wrote:
> Sort alphabetically
Thanks. Will do.

https://reviews.llvm.org/D41373