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

[Ahmed Bougacha via llvm-commits]

ab added inline comments.

================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:93-95
@@ +92,5 @@
+
+  virtual const InstructionSelector *getInstructionSelector() const {
+    return nullptr;
+  }
+
----------------
qcolombet wrote:
> 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.
You're right, I had not considered specializing on optsize.  I'll have to think about the right solution for this.  In the meantime, is yet another FIXME palatable? ;)

================
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.
----------------
qcolombet wrote:
> Should this fixme appear in the IRTranslator instead?
I'm not sure;  I figured, select being the final step, anything that doesn't belong elsewhere ought to be set here.

================
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)) {
----------------
qcolombet wrote:
> 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?
Good idea; added an assert.

================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:60
@@ +59,3 @@
+  }
+#endif
+
----------------
qcolombet wrote:
> 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.
Eh, I'm no fan of either approach, but clearing once is certainly more efficient.

But really, I'm not sure the separate map is the best solution to begin with;  it seems redundant and brittle.  What do you think of querying the MI for the def size?  (or maybe invert that relationship, and keep the type with the vreg info in the first place?).


https://reviews.llvm.org/D22373