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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

The patch looks very sensible to me and should be a performance improvement! I just had a couple of minor 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);
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:312
+      return false;
+    if (!Signed && C.trunc(NewEltSize).zext(EltSize) != C)
+      return false;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130533