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

Tue Jan 19 08:39:20 PST 2021

CarolineConcatto added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10819
+
+  // VECTOR_SHUFFLE doesn't support a scalable mask so use a dedicated node.
+  if (VT.isScalableVector()) {
----------------
david-arm wrote:
> Perhaps worth explaining that we're using VECTOR_SHUFFLE to implement the VECTOR_REVERSE operation for fixed length vectors? It confused me a bit at first trying to understand why we jump suddenly into VECTOR_SHUFFLE that's all.
@david-arm 
I've updated the commit message and the comments as well.
Is that good? 

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5888

+SDValue AArch64TargetLowering::LowerVECTOR_REVERSE(SDValue Op,
+                                                   SelectionDAG &DAG) const {
----------------
david-arm wrote:
> Just for reference, if all the lowering operation does is create the ISD_VECTOR_REVERSE with the same arguments then you can actually just mark the operation as Legal and avoid creating the LowerVECTOR_REVERSE function as it should happen automatically. If we have plans to use SVE for fixed length vectors in future then it might be useful to keep this custom function anyway.
Thank you @david-arm for pointing that.
It is true that we can remove the custom lowering for aarch64 and have it as legal.
This simplifies the code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94883