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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Aditya,

The overall design looks good to me. Obviously given that's a prototype we still miss a bunch of comment, but I like the general approach.
I've put a few nitpicks here and there, but don't pay to much attention to them, I expect you will polish the patch when more people are on board.

Two things I'd like to call out:

1. When making this a real thing, we would need a patch in the GISel doc to explain the design and how it is intended to use
2. I am guessing the PreLegalizerPass is just to show case the prototype, other I think it misses two things: A. The relationship with GICombiner is strange to me and should be reworked IMO (see the comment on the PreLegalizeCombiner class itself) B. The target should have a way to inject/choose its own combines for the pass (I actually believe that if a target needs to do that it would be better of creating its own prelegalize pass)

Anyhow, thanks for putting that up. This looks promising!

Cheers,
-Quentin



================
Comment at: include/llvm/CodeGen/GlobalISel/GICombiner.h:1
+//== ----- llvm/CodeGen/GlobalISel/GICombiner.h --------------------- == //
+//
----------------
Unless it conflicts with something else, I would drop GI in the name. It already lives in the GlobalISel directory.


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


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


================
Comment at: include/llvm/CodeGen/GlobalISel/GICombiner.h:31
+public:
+  GICombiner(GICombinerInfo &CombinerInfo, TargetPassConfig *TPC);
+
----------------
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.


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


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


================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:34
+
+  bool tryCombineCopy(MachineInstr &MI);
+
----------------
Is MI supposed to be a COPY or does this combine will do the check?

Basically, what is the precondition here.


================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerHelper.h:36
+
+  bool tryCombine(MachineInstr &MI);
+};
----------------
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?


================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerInfo.h:28
+  GICombinerInfo(bool AllowIllegalOps, LegalizerInfo *LInfo)
+      : IllegalOpsAllowed(AllowIllegalOps), LInfo(LInfo) {}
+  virtual ~GICombinerInfo() = default;
----------------
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


================
Comment at: include/llvm/CodeGen/GlobalISel/GICombinerInfo.h:33
+  virtual bool combine(MachineInstr &MI, MachineIRBuilder &B,
+                       MachineRegisterInfo &MRI) const = 0;
+};
----------------
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
}


================
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
----------------
The header looks weird


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:262
+    return buildSub(Res, (getRegFromArg(UseArgs))...);
+  }
   MachineInstrBuilder buildSub(unsigned Res, unsigned Op0,
----------------
Unrelated changes?


================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:35
+
+class PreLegalizeCombiner : public MachineFunctionPass {
+public:
----------------
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.


================
Comment at: include/llvm/CodeGen/GlobalISel/PreLegalizeCombiner.h:41
+
+  bool combineMachineFunction(MachineFunction &MF);
+  StringRef getPassName() const override { return "PreLegalizeCombiner"; }
----------------
Unless I missed it, there is no implementation for that.


================
Comment at: lib/CodeGen/GlobalISel/CMakeLists.txt:6
+        GICombinerHelper.cpp
+        PreLegalizeCombiner.cpp
         IRTranslator.cpp
----------------
Sort alphabetically


https://reviews.llvm.org/D41373