[PATCH] D130533: [SVE] Extend findMoreOptimalIndexType so BUILD_VECTORs do not force 64bit indices.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:122
+/// Returns true if the specified node is a vector where all elements can
+/// be truncated to the specified element size without a loss in meaning.
+bool isVectorShrinkable(const SDNode *N, unsigned NewEltSize, bool Signed);
----------------
david-arm wrote:
> I think it's worth adding a comment here saying that this currently only works for BUILD_VECTOR nodes. The name of the function is quite generic, so I think users would probably expect it to also work for things like SPLAT_VECTOR, STEP_VECTOR, etc.
How strongly do you feel about this @david-arm? Personally I see that as an implementation detail rather than something linked to the interface.  It is my hope that this function will be expanded whenever somebody finds a new use case, so there's nothing about this function that is intrinsically linked to `BUILD_VECTOR`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:312
+      return false;
+    if (!Signed && C.trunc(NewEltSize).zext(EltSize) != C)
+      return false;
----------------
david-arm wrote:
> I assume that only one of these cases is tested, right? Either Signed=false or Signed=true, but not both. Is it possible to make sure we have tests for both? If it's too much trouble then don't worry, but I thought maybe it's possible to write a unit test?
I've tried and failed to exercise this condition.  I believe it's not possible with the current usage because `isVectorShrinkable` is only called for i64 based vector types.  The problem is that gathers and scatters only make use of unsigned offsets when the offsets are explicitly zero extended from something that is smaller than i64 and thus by the time that transformation happens no i64 based BUILD_VECTORs exist.  We don't have this problem for signed offsets because that is the default type for `PTR` sized offsets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130533