[PATCH] D102437: [LV] NFC: Decouple foldTailByMasking from isScalarWithPredication.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D102437#2790473 <https://reviews.llvm.org/D102437#2790473>, @fhahn wrote:

> I see, thanks for elaborating! I am not sure how much extra work generating such loops would be, but it seems like disallowing scalarization requires quite a bit of extra complexity as a number of recent patches that introduce additional restrictions show.
>
> The extra checks also seem introduce various TTI checks that seem to mirror the interfaces that compute cost. Do you know how this will impact other targets with scalable vectors? Do you know if scalarization is similarly expensive on other scalable targets? The naive predication/scalarization is also very expensive on most fixed vector targets and almost never profitable, but it simplifies the rest of the code, because LV can generate vector IR for almost all instructions mechanically.

I'll reply to this separately since it's not the only motivation for this patch.

>> I guess it could equally live in CM, but my reasoning to move it was more that it has nothing to do with the cost-model, so moving it to LVL avoids it getting polluted with other CM-specific code in the future.
>
> From the discussion above, it seems to me that it is still more of a cost question rather than a legality question, as IIUC from the discussion, they could be vectorized by scalarizing via a loop? Also, it looks like all uses of the functions moved to LVL are in the cost-model. The replacement of `isLegalMaskedScatter` seems also unrelated and could be submitted separately.

Fair point, I've changed the patch a little bit which in my eyes improves the interfaces and splits out the concerns a bit better by providing interfaces to two distinct questions:

- `requiresPredicatedWidening`: This answers the question "Does this operation require predication when being widened".  I've moved this interface to LVL because it describes a property of the operation and adds a requirement to what capability is needed in order to vectorize it. It does not implement a cost-model choice of how this is eventually widened by the LV. This function was initially coined `isPredicatedInst`, but I find this name a bit more descriptive (also, the original implementation was 'backward', since it used `isScalarWithPredication` to determine whether the operation was predicated). If you feel strongly about the original name, or having this in the CM, I'm happy to change that.
- `isScalarWithPredication`: This answers the question "If this operation needs predication, do we have to fall back on scalarizing it?". This is a LV/cost-model decision, hence why it lives in the CM.

Does this make more sense?

Note that I've committed the changes for `isLegalMaskedScatter|Gather` separately in rG034503e9d2d66ab75679ab5d2ee0848f4de3cac7 <https://reviews.llvm.org/rG034503e9d2d66ab75679ab5d2ee0848f4de3cac7>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102437