[PATCH] D111221: [AArch64][SVE] Improve code generation for VLS i1 masks

[David Truby via Phabricator via llvm-commits]

DavidTruby marked 3 inline comments as done.
DavidTruby added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15737
+static SDValue performSunpkloCombine(SDNode *N, SelectionDAG &DAG) {
+  // sunpklo (mov z, p/z, -1) => mov z, (punpklo p), -1
+  if (N->getOperand(0).getOpcode() == ISD::SIGN_EXTEND &&
----------------
paulwalker-arm wrote:
> This comment doesn't match the code below.  At the DAG level this is really `sunpklo(sext(pred)) -> sext(extract_low_half(pred))`.
> 
> Also, is this transform universally good?  As in, if the later transformation does not occur, is punpklo always better than sunpklo?
I'm not sure that the transform is universally good, however it does significantly simplify the following transform. This transform has the effect of moving the setcc that converts from a i1 register to another register after the sign_extend, which becomes just a single sign extend rather than a chain of unpacks of unknown length. This is a much easier pattern to recognize later.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16438
+      LHS->getOperand(0)->getConstantOperandVal(1) == 0) {
+    auto OrigPred = LHS->getOperand(0)->getOperand(0)->getOperand(0);
+    if (Pred.getOpcode() == AArch64ISD::PTRUE &&
----------------
paulwalker-arm wrote:
> Perhaps I've confused myself with the number of `getOperand()` calls but this doesn't looks quite right:
> ```
> LHS == ISD::SIGN_EXTEND
> LHS->getOperand(0) == ISD::EXTRACT_SUBVECTOR
> LHS->getOperand(0)->getOperand(0) == vector we're extracting the subvector from
> LHS->getOperand(0)->getOperand(0)->getOperand(0) == ???? and how do you know LHS->getOperand(0)->getOperand(0) has an operand to get?
> ```
> 
> I can see the following code then ensures `LHS->getOperand(0)->getOperand(0)->getOperand(0)` is a `PTRUE` but before then depending on `LHS->getOperand(0)->getOperand(0)` the extra call to `getOperand(0)` might assert.  Also, is it really the case that is doesn't matter how the `PTRUE` is processed before being passed to `ISD::EXTRACT_SUBVECTOR`?
I've refactored all of this to make it a bit clearer what's going on, and added an additional check so that we can make sure that `getOperand(0)` actually exists. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111221