[PATCH] D90687: [LV] Ignore VF hint when unsafe

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

`#pragma clang loop vectorize_witdh(..)` ignoring safety checks is indeed bad. Instead of not vectorizing at all in this case, did you consider using `min(UserVF,FeasibleVF)` instead?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5238
+  if (!VectorizerParams::isVFForced() && UserVF &&
+      UserVF > computeFeasibleMaxVF(TC)) {
+    reportVectorizationFailure(
----------------
fhahn wrote:
> dmgreen wrote:
> > I don't think computeFeasibleMaxVF is the maximum safe width. It does a lot of things and is largely based on the backend vector widths. I'm guessing that's why so many tests are changing.
> > 
> > It should probably be based on Legal.getMaxSafeRegisterWidth()?
> > It should probably be based on Legal.getMaxSafeRegisterWidth()?
> 
> computeFeasibleMaxVF should only return legal vectorization factors (using getMaxSafeRegisterWidht) I think. Given that we are free to ignore the hint, if it is not useful, why not just use the largest safe vectorization factors instead of bailing out?
> 
> Unfortunately the handling of UserVF is a bit of a mess, but I think it might be preferable to clamp the UserVF to the maximum vectorization factor in the caller of computeMaxVF where UserVF is actually used.
`Legal.getMaxSafeRegisterWidth()` is called within `computeFeasibleMaxVF`. With `computeFeasibleMaxVF` considering additional architecture concerns, can these be just ignored?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5238
+  if (!VectorizerParams::isVFForced() && UserVF &&
+      UserVF > computeFeasibleMaxVF(TC)) {
+    reportVectorizationFailure(
----------------
Meinersbur wrote:
> fhahn wrote:
> > dmgreen wrote:
> > > I don't think computeFeasibleMaxVF is the maximum safe width. It does a lot of things and is largely based on the backend vector widths. I'm guessing that's why so many tests are changing.
> > > 
> > > It should probably be based on Legal.getMaxSafeRegisterWidth()?
> > > It should probably be based on Legal.getMaxSafeRegisterWidth()?
> > 
> > computeFeasibleMaxVF should only return legal vectorization factors (using getMaxSafeRegisterWidht) I think. Given that we are free to ignore the hint, if it is not useful, why not just use the largest safe vectorization factors instead of bailing out?
> > 
> > Unfortunately the handling of UserVF is a bit of a mess, but I think it might be preferable to clamp the UserVF to the maximum vectorization factor in the caller of computeMaxVF where UserVF is actually used.
> `Legal.getMaxSafeRegisterWidth()` is called within `computeFeasibleMaxVF`. With `computeFeasibleMaxVF` considering additional architecture concerns, can these be just ignored?
With `computeFeasibleMaxVF` already called later, could you store its result for both uses?

I suggest to move `UserVF ? UserVF : computeFeasibleMaxVF(TC)` which is duplicated multiple times below before this condition. Such as:
```
auto MaxVF = computeFeasibleMaxVF(TC);
if (UserVF) {
  if (UserVF > MaxVF) {
    ...
  }
  MaxVF = UserFC;
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90687