Page MenuHomePhabricator

[AARCH64][CostModel] Modified the cost of mask vector load/store
AcceptedPublic

Authored by Miss_Grape on Sep 22 2022, 12:05 AM.

Diff Detail

Event Timeline

Miss_Grape created this revision.Sep 22 2022, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 12:05 AM
Miss_Grape requested review of this revision.Sep 22 2022, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 12:05 AM

I think the change seems sensible because the cost of a NEON load is probably similar to the cost of a SVE masked load on CPUs with SVE. However, I am curious to learn about the problem you're hitting. Are you looking at a specific benchmark or loop where the cost is too high to vectorise with SVE? Or is the cost preventing additional interleaving in the vectoriser?

Matt added a subscriber: Matt.Sep 22 2022, 1:45 PM

I think the change seems sensible because the cost of a NEON load is probably similar to the cost of a SVE masked load on CPUs with SVE. However, I am curious to learn about the problem you're hitting. Are you looking at a specific benchmark or loop where the cost is too high to vectorise with SVE? Or is the cost preventing additional interleaving in the vectoriser?

In the test case of sve-intrinsics-mask-ldst-ext.ll, I found that the instruction of llvm.masked.load.nxv/llvm.masked.store.nxv only corresponds to one assembly, so I think its correct cost should be 1

david-arm accepted this revision.Sep 23 2022, 8:42 AM

In the test case of sve-intrinsics-mask-ldst-ext.ll, I found that the instruction of llvm.masked.load.nxv/llvm.masked.store.nxv only corresponds to one assembly, so I think its correct cost should be 1

OK thanks for the reply. The problem is that the cost isn't always due to the number of instructions, but also the cost in hardware of that instruction. In this particular case though, I think that the SVE ld1/st1 instructions are around the same cost as NEON loads/stores. Also, normal load/store IR nodes get lowered to a SVE ld1/st1, which have a cost of 1. I think this change fixes that inconsistency so I'm happy with it!

LGTM!

This revision is now accepted and ready to land.Sep 23 2022, 8:42 AM
This revision was landed with ongoing or failed builds.Sep 28 2022, 4:41 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 28 2022, 6:22 AM

This broke check-llvm on all my bots:
http://45.33.8.238/linux/87588/step_12.txt
http://45.33.8.238/macm1/45335/step_11.txt
http://45.33.8.238/win/67006/step_11.txt

Also broke the official bots:
https://lab.llvm.org/buildbot/#/builders/109/builds/47509

Please take a look and revert for now if it takes a while to fix.

(And run tests before committing, and watch bots after.)

fhahn added a subscriber: fhahn.Sep 28 2022, 7:36 AM

This broke check-llvm on all my bots:
http://45.33.8.238/linux/87588/step_12.txt
http://45.33.8.238/macm1/45335/step_11.txt
http://45.33.8.238/win/67006/step_11.txt

Also broke the official bots:
https://lab.llvm.org/buildbot/#/builders/109/builds/47509

Please take a look and revert for now if it takes a while to fix.

(And run tests before committing, and watch bots after.)

I went ahead and reverted the commit in eba84971aeefd697f0c0afb96e1c2772745992d9 for now to get the bots back to green, as the test has been failing for a while.

Miss_Grape reopened this revision.Sep 28 2022, 6:51 PM
This revision is now accepted and ready to land.Sep 28 2022, 6:51 PM

Fixed failed test cases

david-arm accepted this revision.Oct 4 2022, 9:30 AM

LGTM!