[PATCH] D22635: GlobalISel: add an actual legalizer

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Tim,

LGTM modulo the name of the extract/sequence things. I don’t think it is a good idea to be close to the subreg world at this point.
See comment inlined.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:155
@@ +154,3 @@
+
+  /// Build and insert `Res0<def>, ... = G_EXTRACT_SUBREGS Ty Src, Idx0, ...`.
+  ///
----------------
I would prefer we stick to extract, extract subreg is highly correlated with sub registers, which is not what we do here.

If you have idea for a different name that would work as well :).
When I come up with the name I had "bitfield extract” in mind. But with can think that as a pack as well or a scatter. Pack/unpack for build_sequence/extract may be good because it shows up the symmetry between the two.

Anyhow, open to suggestion but not g_extract_subregs, it is too close to extract_subreg while being too different.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:158
@@ +157,3 @@
+  /// If \p Ty has size N bits, G_EXTRACT sets \p Res[0] to bits `[Idxs[0],
+  /// Idxs[0] + N)` of \p Src and similarly for subsequent bit-indexes.
+  ///
----------------
In my mind the size was not necessarily the same for all extracted operands.
That being said, we don’t have multiple types for now, nor actual use cases, so this is fine.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:166
@@ +165,3 @@
+
+  /// Build and insert \p Res<def> = G_REG_SEQUENCE \p Ty \p Ops[0], ...
+  ///
----------------
Ditto, stick with G_SEQUENCE, or G_BUILD_SEQUENCE.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:172
@@ +171,3 @@
+  /// \pre setBasicBlock or setMI must have been called.
+  ///
+  /// \return The newly created instruction.
----------------
Add a pre condition that the sum of the size of the arguments must be equal to the size of the \p Ty.

================
Comment at: include/llvm/Target/GenericOpcodes.td:49
@@ +48,3 @@
+// register banks have been selected.
+def G_EXTRACT_SUBREGS : Instruction {
+  let OutOperandList = (outs variable_ops);
----------------
G_EXTRACT.

================
Comment at: include/llvm/Target/GenericOpcodes.td:57
@@ +56,3 @@
+// bit 0).
+def G_REG_SEQUENCE : Instruction {
+  let OutOperandList = (outs unknown:$dst);
----------------
G_SEQUENCE.

================
Comment at: lib/CodeGen/GlobalISel/MachineLegalizePass.cpp:28
@@ +27,3 @@
+char MachineLegalizePass::ID = 0;
+INITIALIZE_PASS(MachineLegalizePass, "legalize-mir",
+                "Legalize the Machine IR a function's Machine IR", false,
----------------
Use DEBUG_TYPE

================
Comment at: lib/CodeGen/GlobalISel/MachineLegalizePass.cpp:49
@@ +48,3 @@
+  // probably want a worklist of instructions rather than naive iterate until
+  // convergence for performance reasons.
+  bool Changed = false;
----------------
This may impact the APIs, so maybe we should settle that now.

================
Comment at: lib/CodeGen/LLVMTargetMachine.cpp:170
@@ +169,3 @@
+
+    PassConfig->addLegalizeMachineIR();
+
----------------
if …
return nullptr;

================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-add.mir:23
@@ +22,3 @@
+    liveins: %q0, %q1, %q2, %q3
+    ; CHECK-DAG: name: test_vector_add
+    ; CHECK-DAG: [[LHS_LO:%.*]](128), [[LHS_HI:%.*]](128) = G_EXTRACT_SUBREGS <2 x s64> %0, 0, 128
----------------
This one shouldn’t use DAG


Repository:
  rL LLVM

https://reviews.llvm.org/D22635