This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Explicitly pass AccessTy to getGEPCost
ClosedPublic

Authored by luke on Jun 22 2023, 9:28 AM.

Details

Summary

Building on D149889, this patch updates SLP to pass the vector type as
the AccessTy to getGEPCost.
This should have the effect of GEPs being costed for more often instead
of being treated as foldable into the address mode and thus free, as
some architectures, notably RISC-V, do not have offset+reg addressing
modes for vector memory accesses.

Note that in SLP, GEPs are costed in two places: getPointersChainCost
and GetGEPCostDiff.

Diff Detail

Unit TestsFailed

Event Timeline

luke created this revision.Jun 22 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 9:28 AM
luke requested review of this revision.Jun 22 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 9:28 AM
luke added inline comments.Jun 22 2023, 9:31 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
1644

Note to self: Should probably get around to deduplicating the getPointersChainCost here and the one in targettransforminfoimpl.h at some point.

reames requested changes to this revision.Jun 28 2023, 10:42 AM

Masking this as request changes to get off review queues until dependent patches have landed. Please "request review" once blocking patches have landed.

I took only a very brief look at this, but I suspect further splitting will be required.

This revision now requires changes to proceed.Jun 28 2023, 10:42 AM
luke planned changes to this revision.Jun 28 2023, 2:57 PM
ABataev added inline comments.Jun 29 2023, 8:25 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
27 ↗(On Diff #533653)

Do you really need this include in the header file?

reames requested changes to this revision.Jun 29 2023, 8:46 AM

Please split the change in lib/Transforms/Scalar to their own review. They aren't connected to SLP, need different reviewers, and don't appear to have any test coverage in this change. They are *not* NFC due to the addition of the accessty inference.

This revision now requires changes to proceed.Jun 29 2023, 8:46 AM
luke added inline comments.Jun 29 2023, 8:47 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
27 ↗(On Diff #533653)

I believe I had to include to use GetElementPtrInst, but now that I think about it maybe it's possible to just use GEPOperator instead.

luke updated this revision to Diff 535843.Jun 29 2023, 9:15 AM

Split out non-SLP changes

luke retitled this revision from [CostModel][SLP] Use getGEPCost AccessTy in more places to [SLP] Explicitly pass AccessTy to getGEPCost.Jun 29 2023, 9:16 AM
luke edited the summary of this revision. (Show Details)

At this point, I'm going defer the remaining review to Alexey. What you've got looks entirely reasonable to me, but Alexey knows the SLP interactions much better.

ABataev accepted this revision.Jun 29 2023, 9:44 AM

Just passing required argument looks good to me.

luke added a comment.Jun 29 2023, 10:34 AM

Just passing required argument looks good to me.

Thanks for the review: Just wanted to double check if you've approved this? I got an email that saying that you accepted the revision, but it still seems to be marked as needs review in Phabricator

The patch is accepted

This revision was not accepted when it landed; it landed in state Needs Review.Jun 29 2023, 11:10 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.