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

[Ahmed Bougacha via llvm-commits]

ab added inline comments.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:34-37
@@ +33,6 @@
+  /// \returns whether selection succeeded.
+  /// \pre  I.getParent() && I.getParent()->getParent()
+  /// \post if returns true: !isPreISelGenericOpcode(I.getOpcode())
+  virtual bool select(MachineInstr &I) const = 0;
+
+protected:
----------------
The XRay perspective is an interesting one.  Have you considered treating this as a legalization problem?  So, you insert your generic instructions at IR->MI translation time; the target declares how it wants to handle them.  If they're not legal, do your common, target-independent, lowering.  Would that make sense?


In general, I'd rather not have selection be "tentative", with fallbacks and whatnot.  I think one shared goal is to be able to definitively tell whether a given instruction will be selectable or not, without actually trying to select it. The hope is to then be able to statically figure out the constructs that "cannot select".

================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:93-95
@@ +92,5 @@
+
+  virtual const InstructionSelector *getInstructionSelector() const {
+    return nullptr;
+  }
+
----------------
Sorry, I misread when I first answered.  Let me try again ;)

Yes, the InstructionSelector is intentionally specific to a subtarget.  Right now (especially for AArch64), there's not much to specialize, but I'm hoping that'll let us separate out feature-specific code  (so instead of a monster TLI, we could maybe have a hierarchy, say avx512>avx>sse, if that makes sense?)

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:64-65
@@ +63,4 @@
+    }
+  }
+
+  // Check that we did select everything. Do this separately to make sure we
----------------
That's an interesting point;  first, do you agree that any of these invariants being violated is a bug? (be it in the legalizer, or in the IR translator, or ...)

The general idea is: an extended machine verifier - specialized for each phase, via MF properties - would enforce these invariant.  Anything coming into the selector has to be selectable.  It's the responsibility of the legalizer (and target-specific transforms, if applicable) to create well-formed instructions by then.


https://reviews.llvm.org/D22373