Page MenuHomePhabricator

[CostModel] Unify getArithmeticInstrCost
ClosedPublic

Authored by samparker on Jun 2 2020, 4:24 AM.

Details

Summary

Add the remaining arithmetic opcodes into the generic implementation of getUserCost and then call this from getInstructionThroughput. Most of the backends have been modified to return the base implementation for cost kinds other RecipThroughput. The outlier here is AMDGPU which already uses getArithmeticInstrCost for all the cost kinds. This change means that most of the opcodes can be removed from that backends implementation of getUserCost.

Diff Detail

Event Timeline

samparker created this revision.Jun 2 2020, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 4:24 AM
dfukalov added inline comments.Thu, Jun 4, 3:26 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
439

Am I right that this reset and TCK_RecipThroughput override in the following call are needed to avoid tests failures? If so, would you please specify errors - I'll try to get a look.

samparker marked an inline comment as done.Fri, Jun 5, 12:40 AM
samparker added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
439

No test failures actually, so shall I just remove this instead?

samparker marked an inline comment as done.Fri, Jun 5, 12:41 AM
samparker added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
439

The RecipThroughput hack does cause a lot of failures, I was wondering whether hoisting some of the generic code here would be a better thing to do?

samparker marked an inline comment as done.Fri, Jun 5, 2:52 AM
samparker added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
439

I tried hoisting from BasicTTI, but the v3 vector cost tests change like this:
-; FASTF64: estimated cost of 6 for {{.*}} fsub <3 x double>
-; SLOWF64: estimated cost of 9 for {{.*}} fsub <3 x double>
+; FASTF64: estimated cost of 8 for {{.*}} fsub <3 x double>
+; SLOWF64: estimated cost of 12 for {{.*}} fsub <3 x double>

dfukalov marked an inline comment as done.Fri, Jun 5, 10:27 AM

AMDGPU part LGTM,
let's wait a couple of days to allow others to take a look

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
442

I see, this override is needed since scheme is changing. I guess I should review this place after you'll complete RFC

dfukalov accepted this revision.Tue, Jun 9, 4:50 PM
This revision is now accepted and ready to land.Tue, Jun 9, 4:50 PM
This revision was automatically updated to reflect the committed changes.

Hi!

I wrote a PR about a crash that starts occuring with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46276