[PATCH] D76640: [GlobalISel] Combine (x - 0) -> x

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:222
+  /// \p C.
+  bool matchConstantOp(const MachineOperand &MOP, int64_t C);
+
----------------
The people on your twitter poll are insane and MO is the clearly dominant abbreviation


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:198
+  (defs root:$root),
+  (match (wip_match_opcode G_SUB):$root,
+    [{ return Helper.matchConstantOp(${root}->getOperand(2), 0); }]),
----------------
Why not also handle add at the same time?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1525
+    return false;
+  auto ValAndVReg = getConstantVRegValWithLookThrough(MOP.getReg(), MRI);
+  return ValAndVReg && ValAndVReg->Value == C;
----------------
arsenm wrote:
> CombinerHelper seems to be reproducing bits that should belong in MIPatternMatch but I guess this case is fine. MIPatternMatch doesn't try to do any look throughs now.
> 
> We should really have an isZeroConstant that will also handle splat vectors
Why not just getConstantVRegVal?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1525-1527
+  auto ValAndVReg = getConstantVRegValWithLookThrough(MOP.getReg(), MRI);
+  return ValAndVReg && ValAndVReg->Value == C;
+}
----------------
CombinerHelper seems to be reproducing bits that should belong in MIPatternMatch but I guess this case is fine. MIPatternMatch doesn't try to do any look throughs now.

We should really have an isZeroConstant that will also handle splat vectors


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir:4-5
+
+...
+---
+name:            right_ident_sub
----------------
... at the end of the function


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

https://reviews.llvm.org/D76640