This is an archive of the discontinued LLVM Phabricator instance.

[LV][SLP] Mark fptosi_sat as vectorizable
ClosedPublic

Authored by dmgreen on Apr 24 2022, 11:26 PM.

Details

Summary

This adds fptosi_sat and fptoui_sat to the list of trivially vectorizable functions, mainly so that the loop vectorizer can vectorize the instruction. Marking them as trivially vectorizable also allows them to be SLP vectorized, and Scalarized.

The signature of a fptosi_sat requires two type overrides (@llvm.fptosi.sat.v2i32.v2f32), unlike other intrinsics that often only take a single. This patch alters hasVectorInstrinsicOverloadedScalarOpd to hasVectorInstrinsicOverloadedOpd, so that it can mark the first operand of the intrinsic as a overleaded (but not scalar) operand.

Diff Detail

Event Timeline

dmgreen created this revision.Apr 24 2022, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 11:26 PM
dmgreen requested review of this revision.Apr 24 2022, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 11:26 PM
spatel added inline comments.Apr 25 2022, 6:11 AM
llvm/include/llvm/Analysis/VectorUtils.h
320–321

Existing typo in the names here and above - "Instrinsic" -> "Intrinsic".
On top of that, these function names don't read clearly to me, so if we're changing things anyway, maybe we can do better.
"isVectorIntrinsicWithScalarOpAtArg"
"isVectorIntrinsicWithDifferentTypeAtArg"

This still seems out-of-place...maybe it should be an introspective function of IntrinsicInst instead - "getTypeForArgNum"?

dmgreen updated this revision to Diff 424957.Apr 25 2022, 10:11 AM

Update function names.

llvm/include/llvm/Analysis/VectorUtils.h
320–321

I believe this function means something like "Given an intrinsic which is isTriviallyVectorizable, but the intrinsic needs more than one type overload in the declaration (i.e not just the return type), this will return true for the operand number of the extra types to add. hasVectorInstrinsicScalarOpd will specify whether it needs to be scalar".

So I believe it's at least somewhat tied with the isTriviallyVectorizable method, and not general enough to handle all the intrinsics that could come up. At least that is my reading of it, it was added for https://reviews.llvm.org/D99439, but has been slightly changed by this patch.

I've renamed the functions, but with the second called isVectorIntrinsicWithOverloadTypeAtArg. Let me know if you think we should move it to IntrinsicInst.

dmgreen added inline comments.Apr 25 2022, 10:15 AM
llvm/include/llvm/Analysis/VectorUtils.h
320–321

For the instrinsics: rG9727c77d58ac

spatel accepted this revision.Apr 25 2022, 1:04 PM

LGTM

llvm/include/llvm/Analysis/VectorUtils.h
320–321

Wow - nice cleanup :)

This revision is now accepted and ready to land.Apr 25 2022, 1:04 PM
fhahn accepted this revision.Apr 26 2022, 1:24 AM

LGTM. thanks!

This revision was landed with ongoing or failed builds.May 3 2022, 1:32 AM
This revision was automatically updated to reflect the committed changes.