[PATCH] D128503: [AArch64][SVE] Lower aarch64_sve_dupq_lane to ld1rq

Mon Jun 27 01:24:54 PDT 2022

peterwaller-arm added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10525-10526
+  if (LD1RQResult) {
+    if (LD1RQResult.getOpcode() == ISD::BUILD_VECTOR)
+      return SDValue();
+    return LD1RQResult;
----------------
As it stands, `tryLowerLD1RQ` is returning the `BUILD_VECTOR` as an in-band signalling mechanism, reusing the 'slot' that would ordinarily be used to communicate the result of the lowering to instead communicate that further lowering should stop for now. This seems hazardous to me.

I'd be interested if other reviewers have an opinion on what is a better way to achieve this. `tryLowerLD1RQ` wants its hands on the address of the load, which begins life as an `ISD::BUILD_VECTOR` and the address is not available until after `BUILD_VECTOR` has been lowered. However, `LowerDUPQLane` after this point can match `Op` and replace it with something else. The logic for converting the `BUILD_VECTOR` to a load cannot be directly called out to since it is within the lowering of `BUILD_VECTOR`, and reimplementing or refactoring that doesn't seem the right way either.

On balance I think the current approach is OK, although I'd prefer to see the signalling be out-of-band, perhaps via a pass-by-pointer bool.

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

https://reviews.llvm.org/D128503