Page MenuHomePhabricator

[AArch64] Add cost tests for bitreverse
ClosedPublic

Authored by Rin on May 19 2021, 2:54 AM.

Details

Summary

This patch includes cost tests for bit reverse as well as some adjustments to the cost model.

Diff Detail

Event Timeline

Rin created this revision.May 19 2021, 2:54 AM
Rin requested review of this revision.May 19 2021, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 2:54 AM
Rin edited the summary of this revision. (Show Details)May 19 2021, 3:02 AM
Rin added reviewers: dmgreen, SjoerdMeijer.
dmgreen added inline comments.May 19 2021, 4:56 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
259

I think that these types should have a slightly higher cost than just 1 (or 1*LT.first, the legalization cost).

It's common in these cost functions to have a table of operand and type, giving a cost to each. That way they can be controlled a little better. Like the getShuffleCost at the end of this file. Would it be worth adding something similar here?

(We could also maybe optimize the v4i32 case better using a vrev32. But it's probably best to use the current cost over what we expect it could be later).

Matt added a subscriber: Matt.May 20 2021, 2:53 AM
Rin updated this revision to Diff 350250.Jun 7 2021, 4:50 AM

Add adequate costs for bitreverse operations

Rin marked an inline comment as done.Jun 7 2021, 4:50 AM
dmgreen added inline comments.Jun 8 2021, 12:34 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
295

Can we change it to only call getTypeLegalizationCost once.

llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll
8 ↗(On Diff #350250)

I see this doesn't give a cost of 2. I think it is trying to use the legal type that i8 will be converted to (i32) for the costs.

Maybe.. can we add something that says "if the original element type is not the same as the legal element type, add 1"?

llvm/test/Analysis/CostModel/AArch64/bitreverse.ll
2

I'm not sure why this testcase was added when there was already one in review adding the same thing. It seems strange. Especially as it missed half of the important cases.

Can you combine the two tests into one? Making sure we have the tests from both. Less boilerplate in them would be good too.

Hey @Rin,
I don't think you need bitreverse-cost-model.ll, you can use only bitreverse.ll.
Why some costs are not being changed in bitreverse.ll?

llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll
2 ↗(On Diff #350250)

Do you need to use

-cost-kind=code-size -mtriple=aarch64-none-eabi

?

I believe you can have the same command as bitreverse.ll, no?

nit:
s/%s --check-prefix=SIZE/ %s /

Rin added a comment.Jun 9 2021, 2:31 AM

Hey @Rin,
I don't think you need bitreverse-cost-model.ll, you can use only bitreverse.ll.
Why some costs are not being changed in bitreverse.ll?

Hi Carol, yeah, you're right, I'll remove it. And I'm working on making the costs adequate as well, which will require adjustments in bitreverse.ll

Rin updated this revision to Diff 350854.Jun 9 2021, 4:40 AM

Address review comments

Rin marked 3 inline comments as done.Jun 9 2021, 4:41 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
295

nit:
s/(LegalisationCost.second)/LegalisationCost.second/

298

Is the '+1' added because the ICA.getID() is informing the type is MVT::i32?
If so, then should we remove these types from the table

{Intrinsic::bitreverse, MVT::i8, 2},
{Intrinsic::bitreverse, MVT::i16, 2}

?
If not(the problem is not with the wrong type being selected on the table) can you add an explanation why the table does not solve the problem?

Rin added inline comments.Jun 9 2021, 5:27 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
298

Is the '+1' added because the ICA.getID() is informing the type is MVT::i32?

If so, then should we remove these types from the table

Yes, that's a good point. I'll remove them.

I think David mentioned the reason why the table does not sole the problem. Because it's trying to use the legal type that i8 will be converted to (i32) for the costs. Which is why I added that if. I'll add a comment in the code to highlight that.

Rin updated this revision to Diff 350869.Jun 9 2021, 5:52 AM

Remove unused typed from cost model table

Rin updated this revision to Diff 350876.Jun 9 2021, 6:13 AM
Rin marked 2 inline comments as done.

Address nit comment

Costs look good, as far as I can tell.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
299

Should this be if (Entry) return LegalisationCost.first * Entry->Cost; now?

Rin updated this revision to Diff 351097.Jun 10 2021, 2:47 AM

Address review comment

Rin marked an inline comment as done.Jun 10 2021, 2:47 AM
Rin added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
299

Yeah, you're right, I have changed it.

dmgreen accepted this revision.Jun 10 2021, 2:58 AM

Thanks. LGTM

This revision is now accepted and ready to land.Jun 10 2021, 2:58 AM
This revision was automatically updated to reflect the committed changes.
Rin marked an inline comment as done.