[PATCH] D72451: [ARM][MVE] Enable extending gathers

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:212-218
+  Root->replaceAllUsesWith(Load);
+  Root->eraseFromParent();
+  if (Root != I)
+    // If this was an extending gather, we need to get rid of the sext/zext
+    // sext/zext as well as of the gather itself
+    I->eraseFromParent();
   LLVM_DEBUG(dbgs() << "masked gathers: successfully built masked gather\n");
----------------
anwel wrote:
> dmgreen wrote:
> > If you moved this "removing" code into tryCreateMaskedGatherOffset or tryCreateMaskedGatherBase it would remove the need for a Root parameter. Not sure if it's cleaner, but it's only tryCreateMaskedGatherOffset that will have a Root, so tryCreateMaskedGatherBase's version will be simpler.
> That would make these few lines a bit simpler for tryCreateMaskedGatherBase, right, but I don't think it's worth the code duplication?
> Because both kinds of gathers could come with a passthru, thus the 5 lines above this dealing with that that build a select would have to be duplicated or made into a new function to be called in tryCreateMaskedGatherBase as well as in tryCreateMaskedGatherOffset.
> That is, whether you just copy or use a new function, around 6 new lines to make it 2 lines simpler in one case?
Ah I see. Because you are updating Load to point to the select. Fair enough. I had missed that.

In that case we should make sure that the Root passed to tryCreateMaskedGatherOffset doesn't change unless the function returns a valid load. At the moment it looks like the function could find an extend (so set Root) but then not find a valid GEP.


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

https://reviews.llvm.org/D72451