[PATCH] D94883: [CodeGen][SelectionDAG]Add new intrinsic experimental.vector.reverse

Fri Jan 22 04:03:44 PST 2021

fhahn added a comment.

In D94883#2515074 <https://reviews.llvm.org/D94883#2515074>, @paulwalker-arm wrote:

> In D94883#2512892 <https://reviews.llvm.org/D94883#2512892>, @david-arm wrote:
>
>> In D94883#2512711 <https://reviews.llvm.org/D94883#2512711>, @fhahn wrote:
>>
>>> Mirroring my comments for D94708 <https://reviews.llvm.org/D94708>: if the intrinsics needs to support fixed vectors, it would be good to have some  tests for platforms other than AArch64 and also support in GlobalISel, which is the default on AArch64 with `-O0` IIRC (or do the transform to shuffles as an IR transform).
>>
>> Hi @fhahn, I agree with your point about -O0, but I'm not sure why we need tests for other platforms? Carol has an extensive set of tests for both fixed width and scalable vectors. The lowering is identical for fixed width vectors regardless of the target so all it would be testing is the different codegen of vector shuffles.
>
> I agree, such extra tests seem wasteful as I'd expect ISD::VECTOR_SHUFFLE to already be sufficiently tested for all targets.

Given the current implementation details that's true, but the implementation could change and having a few tests for other targets guards against the implementation changing in a way that makes it only work on a certain target for example. I'm not saying that is very likely, but something like that could get missed as backends transition to GlobalISel at different times, for example.

I'm not saying we need extensive tests for all targets, just suggesting to add a small sanity test for a different commonly used backend, like X86. Adding such a test seems cheap, but please feel free to ignore the suggestion.

Taking a step back, are there plans to use the intrinsics for fixed vectors?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94883