[PATCH] D85050: [GlobalISel] Combine (logic_op (op x...), (op y...)) -> (op (logic_op x, y))

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1791
+         LogicOpcode == TargetOpcode::G_XOR);
+  MachineIRBuilder MIB(MI);
+  Register Dst = MI.getOperand(0).getReg();
----------------
Shouldn't construct a single use MachineIRBuilder. There should be one in CombinerHelper already


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1820
+    Register LogicOpDst = MRI.createGenericVirtualRegister(MRI.getType(X));
+    auto LogicInst = MIB.buildInstrNoInsert(LogicOpcode)
+                         .addDef(LogicOpDst)
----------------
Why not use the form with all operands in the initial construction?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1842
+    LLT Ty = MRI.getType(X);
+    if (Ty != MRI.getType(Y) || Ty.isVector())
+      return false;
----------------
I don't think vectors are a problem?


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

https://reviews.llvm.org/D85050