[PATCH] D77712: [Target][ARM] Add PerformVSELECTCombine for MVE Integer Ops

[Dave Green via Phabricator via llvm-commits]

dmgreen added a reviewer: efriedma.
dmgreen added a comment.

There is the opposite transform in X86DAGCombiner? Hmm, I'm not sure if that's a great reason but I guess it's fine to put this here. I imagine the X86 code might have been different if the DAGCombine change had worked the other way all along. But it is probably simpler at least to do this here.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:949
 
+  if(Subtarget->hasMVEIntegerOps())
+    setTargetDAGCombine(ISD::VSELECT);
----------------
You can change this.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:11765-11766
+  //
+  // Currently, this is only done for MVE, as it's the only target that benefits
+  // from such a transformation (e.g. VPNOT+VPSEL becomes a single VPSEL).
+
----------------
It's probably best to put a check for if (!Subtarget->hasMVEIntegerOps()) return SDValue(); in case anyone in the future wants to add something Neon here too.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:11768
+
+  // Check if the VSELECT's condition is a XOR.
+  if (N->getOperand(0).getOpcode() != ISD::XOR)
----------------
I don't think this kind of comment adds a lot. It just says what the code says.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:15287
   default: break;
+  case ISD::VSELECT:    return PerformVSELECTCombine(N, DCI, Subtarget);
   case ISD::ABS:        return PerformABSCombine(N, DCI, Subtarget);
----------------
You don't need to change this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77712