[PATCH] D86249: [llvm][sve] Make `llvm.masked.[gather|scatter]` legal for SVE.

Francesco Petrogalli via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 7 08:15:27 PDT 2020


fpetrogalli added inline comments.


================
Comment at: llvm/lib/CodeGen/ScalarizeMaskedMemIntrin.cpp:894
+      // would be required for scalarizing scalable vectors.
+      if (isa<ScalableVectorType>(LoadTy))
+        return false;
----------------
sdesmalen wrote:
> fpetrogalli wrote:
> > efriedma wrote:
> > > sdesmalen wrote:
> > > > Should this check be hoisted out of the switch statement and have it return false if the result or any of the operands to the intrinsic is scalable? The intrinsic itself doesn't really matter, given that the pass doesn't produce loops for any of the loads/stores of scalable vector types, not just gather/scatter.
> > > I doubt anyone is going to care about scalable masked_expandloads anytime soon, but sure, hosting it out makes sense.
> > Given that we know this pass doesn't produce loop, I am happy to hoist this even outside the `if (II)`, and test it on a generic `CallInst` instead of `IntrinsicInst`. Does that make sense? 
> Sure, that sounds fine.
Hi both - sorry for the long wait. I had a go at hoisting the check outside the switch statement, and on a generic CallInst. I actually prefer the original version in this patch, for the following reasons:

1. The check on the generic CallInst is a bit weird, because the pass is called "ScalarizeMaskedMemInstrinsics". Hence, the pass is already expected to ignore anything that is not a masked memory intrinsic, I don't see why we should create CallInst specific behavior. I suggested this, but I don't think it is the right thing to do anymore after seeing a LIT test with a generic call function that is invoking `opt` with `-scalarize-masked-mem-intrin`.
2. Hosting the check outside the switch statement is viable, but than that would make sense if the scalarization process would be something generic over the masked mem intrinsics. Instead, as things are, scalarization is performed on a per intrinsic base. For this reason, I think we should perform this extra test in each case (like it is done in this patch), as we will be able to enable scalarization on each  intrinsic individually without having to revert the generic check for all the masked  memory intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86249



More information about the llvm-commits mailing list