[PATCH] D102607: [AArch64][SVE] Add fixed length codegen for FP_ROUND/FP_EXTEND

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3218-3219
                                              SelectionDAG &DAG) const {
-  if (Op.getValueType().isScalableVector())
+  EVT VT = Op.getValueType();
+  if (VT.isScalableVector())
     return LowerToPredicatedOp(Op, DAG, AArch64ISD::FP_ROUND_MERGE_PASSTHRU);
----------------
This change looks unnecessary.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17842
+  EVT ExtendVT = ContainerDstVT.changeVectorElementType(
+      ContainerSrcVT.getVectorElementType());
+
----------------
This can be `SrcVT`, which when combined with the next comment means `ContainerSrcVT` is no longer required.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17847
+
+  Val = convertToScalableVector(DAG, ContainerSrcVT, Val);
+  Val = getSVESafeBitCast(ExtendVT, Val, DAG);
----------------
I'm surprised this works as `ContainerSrcVT` and `Val` have different element types?  Either way it seems safer to use `ContainerDstVT.changeTypeToInteger()`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17867
+  EVT RoundVT = ContainerSrcVT.changeVectorElementType(
+      ContainerDstVT.getVectorElementType());
+  SDValue Pg = getPredicateForVector(DAG, DL, RoundVT);
----------------
This can be `VT`, which means `ContainerDstVT` is no longer needed.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-fp-extend-trunc.ll:61-67
+; VBITS_GE_512: ptrue [[PG1:p[0-9]+]].h, vl16
+; VBITS_GE_512-NEXT: ld1h { [[OP:z[0-9]+]].h }, [[PG1]]/z, [x0]
+; VBITS_GE_512-NEXT: ptrue [[PG2:p[0-9]+]].s, vl16
+; VBITS_GE_512-NEXT: uunpklo [[UPK:z[0-9]+]].s, [[OP]].h
+; VBITS_GE_512-NEXT: fcvt [[RES:z[0-9]+]].s, [[PG2]]/m, [[UPK]].h
+; VBITS_GE_512-NEXT: st1w { [[RES]].s }, [[PG1]], [x1]
+; VBITS_GE_512-NEXT: ret
----------------
In general for the fixed length arith tests we've added `VBITS_EQ_256` CHECK lines to the 512bit tests, just to show sensible type legalisation (see sve-fixed-length-fp-reduce.ll).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102607