[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:986
+  auto *WideCanonicalIV =
+      cast<VPWidenCanonicalIVRecipe>(*FoundWidenCanonicalIVUser);
+  // Walk users of WideCanonicalIV and replace all compares of the form
----------------
fhahn wrote:
> ABataev wrote:
> > fhahn wrote:
> > > ABataev wrote:
> > > > fhahn wrote:
> > > > > ABataev wrote:
> > > > > > shiva0217 wrote:
> > > > > > > There is a case that VPWidenCanonicalIVRecipe didn't be generated with tail folding.
> > > > > > > 
> > > > > > >     int i;
> > > > > > >     int foo (int q, int z)
> > > > > > >     {
> > > > > > >       int e = 0;
> > > > > > >       while (z < 1)
> > > > > > >         {
> > > > > > >           q = z * 2;
> > > > > > >           if (q != 0)
> > > > > > >             for (i = 0; i < 2; ++i)
> > > > > > >               e = 5;
> > > > > > >           ++z;
> > > > > > >         }
> > > > > > >       return e;
> > > > > > >     }
> > > > > > > 
> > > > > > > `for (i = 0; i < 2; ++i)` been simplifed as `store i32 2, ptr @i`.
> > > > > > > Both pointer and store value are loop-invariant, so the mask(VPWidenCanonicalIVRecipe) might not be generated.
> > > > > > > Should we suppress the replacement when the mask is not available?
> > > > > > Fixed, added the test
> > > > > Was this fixed by adding the ` bool KeepVPCanonicalWidenRecipes` flag? What's the test case for this? There's a new `no_masking` case, but it has an empty body and no vector code is generated? 
> > > > > 
> > > > 1. Yes.
> > > > 2. Yes, this test is a reduced version of the failed case
> > > Is it possible it is over-reduced? The same IR seems to be generated both with and without `KeepVPCanonicalWidenRecipes` IIUC, because the loop is not vectorized due to being empty. 
> > > 
> > > What's the issue if there's no mask/canonical widen recipe? Wouldn't it be fine to jus not replace anything?
> > When I checked it, it crashed without this parameter, maybe there were some other changes.
> Can you check if it still crashes? Would be good to understand exactly what the issue is, and if possible avoid having a separate `KeepVPCanonicalWidenRecipes` flag
`KeepVPCanonicalWidenRecipes` might be motivated by the case that VPWidenCanonicalIVRecipe once exist but been optimized out to the (VPWidenIntOrFpInductionRecipe IV ule trip-count) which let replaceHeaderPredicateWithIdiom fail to replace (VPWidenCanonicalIV ule trip-count) to all-true mask.

    void foo (char *a) {
      for (int i = 0; i < 256; i++)
        if (i != '\n')
          a[i] = 0;
    }



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750