[PATCH] D106561: [AArch64] Optimise min/max lowering in ISel

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7132
+  SDValue Cond = DAG.getSetCC(DL, VT, Op0, Op1, CC);
+  return DAG.getSelect(DL, VT, Cond, Op0, Op1);
+}
----------------
efriedma wrote:
> If I'm understanding correctly, if we mark both ISD::SMAX and ISD::VSELECT as "Expand", vector legalization decides to unroll it.  So we mark ISD::SMAX as "Custom", then explicitly lower to a VSELECT to get the code we want.
> 
> This seems kind of silly, given VSELECT is equivalent to AArch64ISD::BSP.  For the sake of making changes to target-independent code easier, maybe we should consider marking ISD::VSELECT "custom"?  Or add a target hook to indicate whether the operation is cheap?
Yes, we didn't mention that anywhere here. There are three ways we thought of fixing this - either make vselect custom/legal, adding a target hook for the expand code or custom lowering the type. Because it was only a single type, custom lowering seemed like the simplest route forward. The target hook felt messy for one type on one operation, and custom lowering vselect could be a much larger change - one that would easily cause regressions if a lot of other transforms were not added.

I guess this turned into more code than I expected, with it being mixed up with SVE lowering. Perhaps custom lowering vselect would be better in the long run if someone was to optimize the BSP's in all the cases it would need, but this patch seems fine to me for the problem it is solving.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106561