Page MenuHomePhabricator

[AMDGPU] Tune perfhint analysis to account access width
ClosedPublic

Authored by rampitec on Jul 8 2021, 12:31 PM.

Details

Summary

A function with less memory instructions but wider access
is the same as a function with more but narrower accesses
in terms of memory boundness. In fact the pass would give
different answers before and after vectorization without
this change.

Diff Detail

Event Timeline

rampitec created this revision.Jul 8 2021, 12:31 PM
rampitec requested review of this revision.Jul 8 2021, 12:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 12:31 PM
Herald added a subscriber: wdng. · View Herald Transcript
foad added inline comments.Jul 15 2021, 3:36 AM
llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp
222–223

The "InstCount" names seem like a lie since we are no longer counting the number of instructions, but I don't have a better idea.

rampitec added inline comments.Jul 15 2021, 9:14 AM
llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp
222–223

Yes, I also didn't come up with a better name. In fact it always was a lie since IR instruction is not a HW instruction. All of that was inspired by the desire to move pass later in the pipeline and then I realized to preserve its behavior I need to adjust it for LD/ST combining. But the metric name has drifred even more from reality as it used to be.

yaxunl added inline comments.Jul 15 2021, 10:03 AM
llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp
222–223

How about Count => Cost ?

Also, we need to do the same change to IAMInstCoount and LSMInstCount to keep the way to calculate the cost consistent.

rampitec added inline comments.Jul 15 2021, 10:25 AM
llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp
222–223

Thanks, cost sounds better.

rampitec updated this revision to Diff 359087.Jul 15 2021, 12:09 PM
rampitec marked 2 inline comments as done.
  • Renamed variables to read 'cost' instead of 'count'.
  • Adjusted IAM and LSM costs accordingly.

I still think this really should be a machine analysis

I still think this really should be a machine analysis

Probably, but that's really a completely separate change.

yaxunl accepted this revision.Jul 21 2021, 12:05 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 21 2021, 12:05 PM
This revision was landed with ongoing or failed builds.Jul 21 2021, 12:46 PM
This revision was automatically updated to reflect the committed changes.