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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Ahmed,

Looks mostly good to me. Couple of inlined comment. Mainly, I believe we can avoid the ifdef in MRI::setRegClass and the getInstructionSelect may live somewhere else and take an additional parameter.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:35
@@ +34,3 @@
+  /// \pre  I.getParent() && I.getParent()->getParent()
+  /// \post if returns true: !isPreISelGenericOpcode(I.getOpcode())
+  virtual bool select(MachineInstr &I) const = 0;
----------------
More accurately, I has been replaced by a sequence of instructions where this condition applies.
But I can live with that shortcut :).

================
Comment at: include/llvm/CodeGen/TargetPassConfig.h:229
@@ -228,1 +228,3 @@
 
+  virtual bool addGlobalInstructionSelect() { return true; }
+
----------------
Add a comment for that method.
Also add an additional hook for inject PreGlobalInstructionSelect passes. (Could be a follow-up patch.)

================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:93-95
@@ +92,5 @@
+
+  virtual const InstructionSelector *getInstructionSelector() const {
+    return nullptr;
+  }
+
----------------
ab wrote:
> 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?)
FWIW, that’s how I envisioned the thing, that being said, maybe we should have a different interface elsewhere, like getInstructionSelector(MF)?
The idea would be to have the avx512>avx etc. distinction but also things that depends on the function attribute like optimize for code size. The MF allows us to access the function attributes and the sub target.

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:51
@@ +50,3 @@
+  // FIXME: freezeReservedRegs is now done in IRTranslator, but there are many
+  // other MF/MFI fields we need to initialize.
+  // FIXME: We could introduce new blocks and will need to fix the outer loop.
----------------
Should this fixme appear in the IRTranslator instead?

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:52
@@ +51,3 @@
+  // other MF/MFI fields we need to initialize.
+  // FIXME: We could introduce new blocks and will need to fix the outer loop.
+  for (MachineBasicBlock *MBB : post_order(&MF)) {
----------------
Could you assert on an unchanged number of block within the loop or something, so that we know when we have to properly handle the case?

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:69
@@ +68,3 @@
+  // FIXME: This (and other checks) should move into a verifier.
+  // FIXME: That would also let us selectively enable it.
+  for (MachineBasicBlock &MBB : MF) {
----------------
Agreed on all the FIXMEs, plus we should set a property after-isel on the function. That’s how we will selectively enable the new checks.

================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:60
@@ +59,3 @@
+  }
+#endif
+
----------------
I would just clear the whole map at the end of the select pass. Will avoid the ifdef and the cost for the extract logic.


https://reviews.llvm.org/D22373