[PATCH] D120891: [AArch64] Perform first active true vector combine

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14376
+                                  const AArch64Subtarget *Subtarget) {
+  if (!Subtarget->hasSVE() || !DCI.isBeforeLegalize())
+    return SDValue();
----------------
Sorry I missed this previously but I think this is bug and should be
`if (!Subtarget->hasSVE() || DCI.isBeforeLegalize())` as otherwise you might create a `PTEST` with illegal types, which cannot be legalised and will instead cause a compiler crash.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14400
+                                              const AArch64Subtarget *Subtarget) {
+  assert(N->getOpcode() == ISD::EXTRACT_VECTOR_ELT);
+  if (SDValue Res = performFirstTrueTestVectorCombine (N, DCI, Subtarget))
----------------
I think it's worth having this same assert inside `performFirstTrueTestVectorCombine` just in case it gets called via a different route in the future.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14401
+  assert(N->getOpcode() == ISD::EXTRACT_VECTOR_ELT);
+  if (SDValue Res = performFirstTrueTestVectorCombine (N, DCI, Subtarget))
+    return Res;
----------------
Unwanted space?


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

https://reviews.llvm.org/D120891