[PATCH] D101544: [AArch64][SVE] Improve sve.convert.to.svbool lowering

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3684
+  EVT OutVT = Op.getValueType();
+  EVT InVT = Op.getOperand(1).getValueType();
+
----------------
This is used five times within the function so perhaps worth having `SDValue InOp`?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3701
+    unsigned Intr =
+        cast<ConstantSDNode>(Op.getOperand(1).getOperand(0))->getZExtValue();
+    if (Intr == Intrinsic::aarch64_sve_ptrue)
----------------
Can you use `getConstantOperandVal` here?


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-reinterpret.ll:78
+; Reinterpreting a ptrue should not introduce an `and` instruction.
+; CHECK-LABEL: reinterpret_ptrue:{{.*$}}
+; CHECK: ptrue
----------------
If this required? Same comment for reinterpret_cmpgt.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-reinterpret.ll:79
+; CHECK-LABEL: reinterpret_ptrue:{{.*$}}
+; CHECK: ptrue
+; CHECK-NEXT: ret
----------------
I think you can be more precise here as we are expecting `ptrue p0.h`


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-reinterpret.ll:90
+; CHECK: cmpgt
+; CHECK-NOT: and
+; CHECK-NEXT: ret
----------------
Is this required?  I would have thought we can check for the precise output.  If you pass the mask as a parameter I think we'd effectively have:
```
CHECK: cmpgt p0/z, z0.h, z1.h
CHECK-NEXT: ret
```
In fact there's a good shout here for just using `llvm/utils/update_llc_test_checks.py` to generate the check lines for this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101544