[PATCH] D42012: [GlobalISel][TableGen] Add support for SDNodeXForm

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with some nits fixed



================
Comment at: include/llvm/Target/GlobalISel/Target.td:50-51
+
+// Defines a custom renderer. This is analogous to SDNodeXForm from
+// SelectionDAG.
+//
----------------
We should probably expand on this to say that it matches an MI and renders directly to the result instruction (without an intermediate node unlike SDNodeXForm)


================
Comment at: include/llvm/Target/GlobalISel/Target.td:58
+  // the specified instruction. It should be of the form:
+  //   void render(MachineInstrBuilder &MIB, const MachineInstr &I)
+  string RendererFn = rendererfn;
----------------
This is a tiny nitpick, but MachineInstr variables are usually named MI


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:682
+def gi_trunc_imm : GICustomOperandRenderer<"renderTruncImm">,
+                GISDNodeXFormEquiv<trunc_imm>;
+
----------------
Indentation


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:95
 
+  void renderTruncImm(MachineInstrBuilder &MIB, const MachineInstr &I) const;
+
----------------
I -> MI


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1528
+void AArch64InstructionSelector::renderTruncImm(MachineInstrBuilder &MIB,
+                                                const MachineInstr &I) const {
+  const MachineRegisterInfo &MRI = I.getParent()->getParent()->getRegInfo();
----------------
I -> MI


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3707-3721
   std::sort(ComplexPredicates.begin(), ComplexPredicates.end(),
             [](const Record *A, const Record *B) {
               if (A->getName() < B->getName())
                 return true;
               return false;
             });
+
----------------
Given that these lambdas are the same, could we unify them in a 'orderByName' lambda or similar?


https://reviews.llvm.org/D42012