[PATCH] D133257: [GISel] Fix match tree emitter.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

It would be good to add a dedicated tablegen test for this checking the generated code. I find them helpful when trying to extend tablegen without having to make some manageable sized extract to start from



================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:534
-  (match (wip_match_opcode G_FNEG):$root,
-         [{ return Helper.matchCombineFNegOfFNeg(*${root}, ${matchinfo}); }]),
-  (apply [{ return Helper.replaceSingleDefInstWithReg(*${root}, ${matchinfo}); }])
----------------
Should also delete matchCombineFNegOfFNeg


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:559
-  (match (wip_match_opcode G_FABS):$root,
-         [{ return Helper.matchCombineFAbsOfFAbs(*${root}, ${matchinfo}); }]),
-  (apply [{ return Helper.replaceSingleDefInstWithReg(*${root}, ${matchinfo}); }])
----------------
Should also delete matchCombineFAbsOfFAbs


================
Comment at: llvm/utils/TableGen/GlobalISel/CodeExpansions.h:27-28
   void declare(StringRef Name, StringRef Expansion) {
-    bool Inserted = Expansions.try_emplace(Name, Expansion).second;
-    assert(Inserted && "Declared variable twice");
-    (void)Inserted;
+    // Duplicates are not inserted. The expansion refers to different operands
+    // but to the same virtual register.
+    Expansions.try_emplace(Name, Expansion);
----------------
Comment doesn't read right. Maybe you meant "refers not to different operands"?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133257/new/

https://reviews.llvm.org/D133257