[PATCH] D22373: [GlobalISel] Introduce a simple instruction selector.

[Ahmed Bougacha via llvm-commits]

ab added a comment.

In https://reviews.llvm.org/D22373#484484, @MatzeB wrote:

> I just looked at the MachineRegisterInfo parts. This is also a highlevel comment, not necessarily about this patch (which just continues the current direction): It seems to me like generic regs and "normal" vregs don't have that much in common, they mostly share the same kind of MachineOperand and the same def/use lists. Apart from that they contain different information (register bank + size vs. register class). Wouldn't it make sense to introduce a new MachineOperand kind for them and separate them from "normal" vregs?


I agree;  I had a few ideas that I wrote down in the MRI FIXME.  The one that I currently prefer is to have a type for gvregs, a different one for vregs (so, no 'unsigned' anymore), and overload the relevant parts of MRI/... based on that.  WDYT?


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:32-35
@@ +31,6 @@
+  ///
+  /// \returns whether selection succeeded.
+  /// \pre  I.getParent() && I.getParent()->getParent()
+  /// \post !isPreISelGenericOpcode(I.getOpcode())
+  virtual bool select(MachineInstr &I) const = 0;
+
----------------
dberris wrote:
> Is a boolean sufficient for conveying the failure scenario for selection?
> 
> How do specific instruction selections convey the nuance of the failure? Could there be multiple passes on the same function caused by a failure to select -- maybe because there's not enough information yet?
> 
> For example, if an instruction is ignored by the selector, or replaced by a no-op, does it have to return failure? What will that cause the `InstructionSelect` pass to do? I'm not sure if that documentation has to happen in this interface or in the `InstructionSelect` pass instead.
The way I see it, the contract is very simple: after select(I) succeeds, 'I' cannot be generic anymore, and we cannot have introduced other generic instructions.  Anything else is a failure from which we cannot recover.

So, specifically, if a non-generic instruction is ignored or deleted, all is well.  But a generic instruction cannot be ignored, only deleted.  Does that make sense?


If you're imagining things like:

    %a = foo
    %b = bar %a

and (ignoring ordering) failing to select %a, but being able to select %b:  I don't think that's acceptable, the assumption being that every instruction at selection time is selectable.

We talked about introducing some sort of verifier for this, but I don't have a full answer to that yet.

================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:93-95
@@ +92,5 @@
+
+  virtual const InstructionSelector *getInstructionSelector() const {
+    return nullptr;
+  }
+
----------------
echristo wrote:
> Do you really intend for this to be a subtarget feature?
No, and I don't think the other GISel passes should be either, only the *Info/Selector classes should.  Do you agree?

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:62-63
@@ +61,4 @@
+      MachineInstr &MI = *MII++;
+      if (!ISel->select(MI))
+        reportSelectionError(MI, "Cannot select");
+    }
----------------
dberris wrote:
> Somewhat related to my question above on the `InstructionSelector` interface -- maybe we should delegate the selection error messaging to the implementation?
Hmm, I figured that would pretty much duplicate the exact same error message in each implementation.  Do you envision useful differences?

================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:103
@@ +102,3 @@
+
+  assert(STI.getInstrInfo() && "Subtarget should have TargetInstrInfo!");
+  const TargetInstrInfo &TII = *STI.getInstrInfo();
----------------
echristo wrote:
> These seem a bit ... overly assert-y.
Isn't it?  I changed it to pass those when constructing the selector in the subtarget;  there's more awkwardness involved there (RBI vs Subtarget), would appreciate another look!

================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:143
@@ +142,3 @@
+
+    I.setDesc(TII.get(NewOpc));
+    // FIXME: Should the type be always reset in setDesc? If we move the type
----------------
t.p.northover wrote:
> From here on seems like it would be better off outside the switch. It applies to the vast majority of instructions.
My gut feeling was to avoid generalizing for now, and make sure that better abstractions emerge as we support more instructions.

But for this specific piece of logic, I agree it's probably going to be very common.  I extract it out into a "fixup operands" helper, let me know what you think.

================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:155
@@ +154,3 @@
+    // number and order of operands is the same pre- and post-isel.
+    for (unsigned OpI = 0, OpE = I.getNumOperands(); OpI != OpE; ++OpI) {
+      MachineOperand &MO = I.getOperand(OpI);
----------------
t.p.northover wrote:
> Do we have a policy on implicit ops on generic `MachineInstrs`? I'm guessing we don't expect there to be any, but if not we might want `getNumExplicitOperands` instead.
I assumed we don't get implicit ops, but it's good that you mention that: I added an assert for now; we can revisit if we end up with a good reason to use them.


https://reviews.llvm.org/D22373