This is an archive of the discontinued LLVM Phabricator instance.

[TTI] Change getOperandsScalarizationOverhead to take Type args
ClosedPublic

Authored by dmgreen on Feb 8 2021, 12:43 PM.

Details

Summary

As a followup to D95291, getOperandsScalarizationOverhead was still using a VF as a vector factor if the arguments were scalar, and would assert on certain matrix intrinsics with differently sized vector arguments. This patch removes the VF arg, instead passing the Types through directly. This should allow it to more accurately compute the cost without having to guess at which operands will be vectorized, something difficult with more complex intrinsics.

Diff Detail

Event Timeline

dmgreen created this revision.Feb 8 2021, 12:43 PM
dmgreen requested review of this revision.Feb 8 2021, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 12:43 PM
sdesmalen added inline comments.Feb 9 2021, 3:29 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
629–631

nit: if(auto *VecTy = dyn_cast<VectorType>(Ty))

651

Is this not doing something different than it did before, i.e. it now always uses InTy for each of the arguments to get the cost, where it previously used the types from each of the arguments?
I expected something similar to what you did in LoopVectorize with MaybeVectorizeType.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
752–753

nit: clang-format? (over 80char I think)

dmgreen updated this revision to Diff 322335.Feb 9 2021, 3:54 AM
dmgreen marked 2 inline comments as done.

Thanks for taking a look.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
651

Yeah. This version of getScalarizationOverhead is only called from getArithmeticInstrCost (or ARMTTIImpl::getArithmeticInstrCost etc). So the arguments/return value are all likely to all be the same value, and this is making that assumption. It is probably at least good enough as a heuristic, when compared to the else below. I've added a comment too.

sdesmalen added inline comments.Feb 9 2021, 2:53 PM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
651

If this code assumes all arguments are of the same type, can you add an assert which checks this?

dmgreen added inline comments.Feb 11 2021, 4:03 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
651

Unfortunately that isn't something that is always true. The type of Args may be incorrect, even the scalar type. The vectorizer can do bitwidth narrowing at the same time as vectorization, in which case the Ty will be the most accurate type available. If the original Arg type was an i32, but the vectorizer is vectorizing with it as an i8 to maximize the vector factor, a vXi8 is the correct type to use. That will be Ty for all the uses of this function.

I could try to move this array creation to the callers of the function. That would involve duplicating this bit of code in a few places, but after e771614bae0a05 removed the only other use of it, it should ensure these types are correct from the outside.

dmgreen updated this revision to Diff 322946.Feb 11 2021, 4:22 AM

Pass Tys array to getScalarizationOverhead

Thanks for the changes! I think moving the creation of the Type array to the callers is an improvement, it makes it more clear what types are used to calculate the cost.
Just a few more nits, and then I'm happy.

llvm/include/llvm/Analysis/TargetTransformInfo.h
725–727

Comment needs updating.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
637

nit: can you add a comment to describe the meaning of the arguments?

Just my mental understanding:

  • InTy is the type of the vector(ized) node that needs scalarizing, hence it's always a VectorType.
  • Args are the arguments to the node. These arguments are passed to see e.g. if any of them is a constant. If the question originates from the loopvectorizer, then the node and arguments are likely all vectorized with type InTy. Otherwise, they may have other types (such as the matrix intrinsic), but that information cannot be deduced from the arguments themselves yet, because the vectorization may not have happened yet.
  • Tys are the actual types of each of the arguments after vectorizing, and are used to calculate the scalarization cost.
651

Thanks for the explanation and changes!

nit: This comment ("// Assume all arguments are of type Ty, ..") can now be removed.

dmgreen updated this revision to Diff 323374.Feb 12 2021, 9:47 AM

Update comments.

LGTM, thanks @dmgreen!

llvm/include/llvm/Analysis/TargetTransformInfo.h
727

nit: \p Tys

llvm/include/llvm/CodeGen/BasicTTIImpl.h
639

nit: \p RetTy, \p Args and \p Tys

sdesmalen accepted this revision.Feb 18 2021, 8:57 AM
This revision is now accepted and ready to land.Feb 18 2021, 8:57 AM