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

Anna Welker via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 14 04:00:18 PST 2020


anwel marked 7 inline comments as done.
anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:129
   if (GEPPtr->getType()->isVectorTy()) {
-    LLVM_DEBUG(dbgs() << "masked gathers: gather from a vector of pointers"
-                      << " hidden behind a getelementptr currently not"
-                      << " supported. Expanding.\n");
+    //LLVM_DEBUG(dbgs() << "masked gathers: gather from a vector of pointers"
+    //                  << " hidden behind a getelementptr currently not"
----------------
dmgreen wrote:
> This can be left in I would guess? Or removed entirely if it's too noisy. Maybe reword it to "can't find gep" as opposed to "expanding".
I think I will remove it completely, to get rid of some noise. With the current implementation, it is not clear anymore whether we will fail (i.e., expand) or not after this point. For types for which we can build masked gathers from a vector of pointers, we are now able to transform this, as demonstrated by the following test:
```
define arm_aapcs_vfpcc <4 x i32> @qi4(<4 x i32*> %p) {
; CHECK-LABEL: qi4:
; CHECK:       @ %bb.0: @ %entry
; CHECK-NEXT:    vmov.i32 q1, #0x10
; CHECK-NEXT:    vadd.i32 q1, q0, q1
; CHECK-NEXT:    vldrw.u32 q0, [q1]
; CHECK-NEXT:    bx lr
entry:
  %g = getelementptr inbounds i32, <4 x i32*> %p, i32 4
  %gather = call <4 x i32> @llvm.masked.gather.v4i32.v4p0i32(<4 x i32*> %g, i32 4, <4 x i1> <i1 true, i1 true, i1 true, i1 true>, <4 x i32> undef)
  ret <4 x i32> %gather
}```


================
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");
----------------
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?


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:266
+    Root = dyn_cast<Instruction>(*I->users().begin());
+    if (isa<SExtInst>(Root)) {
+      LLVM_DEBUG(dbgs() << "masked gathers: found an extending gather\n");
----------------
dmgreen wrote:
> I think we have to check the type of the extend, otherwise it might be transforming to any odd thing.
> 
> Make sure there is some testcase for this too, I don't think we have maybe tests for stranger types. i64 are a good candidate. Even they are not legal (for the instruction versions that we are dealing with here), they can quite easily be created by the vectorizer.
Thanks for spotting that, I wrote a test and discovered that it crashed the compiler. I'll upload a new diff with this fixed.


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

https://reviews.llvm.org/D72451





More information about the llvm-commits mailing list