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

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:22
@@ +21,3 @@
+
+#define DEBUG_TYPE "instructionselect"
+
----------------
instruction-select

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:27
@@ +26,3 @@
+char InstructionSelect::ID = 0;
+INITIALIZE_PASS_BEGIN(InstructionSelect, "instructionselect",
+                      "Select target instructions out of generic instructions",
----------------
arsenm wrote:
> You can use INITIALIZE_PASS instead of repeating BEGIN/END
Use DEBUG_TYPE macro

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:27-32
@@ +26,8 @@
+char InstructionSelect::ID = 0;
+INITIALIZE_PASS_BEGIN(InstructionSelect, "instructionselect",
+                      "Select target instructions out of generic instructions",
+                      false, false);
+INITIALIZE_PASS_END(InstructionSelect, "instructionselect",
+                    "Select target instructions out of generic instructions",
+                    false, false);
+
----------------
You can use INITIALIZE_PASS instead of repeating BEGIN/END

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:43-44
@@ +42,4 @@
+  Err << Message << ":\n"
+      << "In function: " << MF.getName() << "\n"
+      << MI << "\n";
+  report_fatal_error(Err.str());
----------------
Single quotes around '\n'

================
Comment at: lib/CodeGen/GlobalISel/InstructionSelect.cpp:71-72
@@ +70,4 @@
+  // FIXME: That would also let us selectively enable it.
+  for (MachineBasicBlock &MBB : MF)
+    for (MachineInstr &MI : MBB)
+      if (isPreISelGenericOpcode(MI.getOpcode()))
----------------
Braces

================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:47
@@ +46,3 @@
+  if (SizeIt != SizeMap.end()) {
+    // FIXME: Can banks have sizes that aren't multiples of 8?
+    assert(SizeIt->second == (RC->getSize() * 8) &&
----------------
1 bit condition registers?

================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:88
@@ +87,3 @@
+bool AArch64InstructionSelector::select(MachineInstr &I) const {
+  DEBUG(dbgs() << "AArch64: select\n");
+
----------------
I don't think targets should need to have their own DEBUG here

================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:157
@@ +156,3 @@
+      MachineOperand &MO = I.getOperand(OpI);
+      DEBUG(dbgs() << "Converting operand: " << MO << "\n");
+
----------------
Single quotes around '\n'


https://reviews.llvm.org/D22373