[PATCH] D102607: [AArch64][SVE] Add fixed length codegen for FP_ROUND/FP_EXTEND
Paul Walker via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 20 10:28:57 PDT 2021
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
More information about the llvm-commits
mailing list