[PATCH] D114580: [AArch64][SVE] Mark fixed-type FP extending/truncating loads/stores as custom

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1545
+  if (VT.isFloatingPoint()) {
+    MVT InnerVT = VT.changeVectorElementType(MVT::i8);
+    while (InnerVT != VT) {
----------------
This looks weird to me, shouldn't InnerVT be floating point? I guess the reason this works is because the i8 case is essentially bogus and you end up with the necessary floating point types from the second iteration.  Either way I think this wants to be `MVT::f16`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18600
+
+  if (ContainerVT.getVectorElementType().isFloatingPoint() &&
+      Load->getExtensionType() != ISD::NON_EXTLOAD) {
----------------
Purely to help with formatting I believe you can just do `VT.isFloatingPoint()` here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18601
+  if (ContainerVT.getVectorElementType().isFloatingPoint() &&
+      Load->getExtensionType() != ISD::NON_EXTLOAD) {
+    LoadVT = ContainerVT.changeTypeToInteger();
----------------
Can you be specific here as in use `== ISD::EXTLOAD` as we should need to support any other extension types.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18611
+
+  if (LoadVT != ContainerVT) {
+    NewLoad = getSVESafeBitCast(ExtendVT, NewLoad, DAG);
----------------
I think it'll be safer to mirror the other if condition as the two are linked (i.e. we need to do both or none based on the same requirement).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114580