[PATCH] D117900: [AArch64][SVE] Fold gather/scatter with 32bits when possible

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16367-16370
+static bool findMoreOptimalIndexType(const MaskedGatherScatterSDNode *N,
+                                     SDValue &BasePtr, SDValue &Index,
+                                     ISD::MemIndexType &IndexType,
+                                     SelectionDAG &DAG) {
----------------
Hi @CarolineConcatto, sorry for the too late review comment but I think this function has a serious problem albeit one that isn't going to trigger with its current usage.

There are several places where the function returns false after having already changed some of its input reference parameters (i.e. BasePtr).  This is unnatural and likely to confuse the unaware.  From the original work you'll see that these parameters were only changed when something more optimal is found.

With the current format of this function callers will have to backup the parameters, which is just something they're not going to know to do until they've spent time debugging why their "local" state has been trashed.

Can you please change the function so that the reference inputs are unchanged when returning false?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117900