This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add cost modelling for masked memory op
ClosedPublic

Authored by alextsao1999 on Jan 21 2022, 5:27 AM.

Diff Detail

Event Timeline

alextsao1999 created this revision.Jan 21 2022, 5:27 AM
alextsao1999 requested review of this revision.Jan 21 2022, 5:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 5:27 AM
kito-cheng added inline comments.Jan 24 2022, 6:48 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
173

That seems like not included the memory access cost? how about return getMemoryOpCost (Opcode, Src, Alignment, AddressSpace, CostKind) here?

alextsao1999 added inline comments.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
173

Thanks, I fixed it

frasercrmck added inline comments.Jan 25 2022, 1:53 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
169

How come we're not adding support for fixed-length vectors at the same time? The commit title/description should make it clear that this is scalable-only.

llvm/test/Analysis/CostModel/RISCV/masked_ldst.ll
28

What do you mean by "legal" here? The test isn't testing a configuration in which fixed-length vectors are supported by the backend.

48

Again, what's illegal about these types?

93

This comment says that these types are legal, but the expected output is that there's an "Invalid cost"? That doesn't sound quite right.

101

Again, what's illegal about these?

  • Remove misunderstood words
alextsao1999 edited the summary of this revision. (Show Details)Jan 25 2022, 9:21 AM
alextsao1999 added inline comments.Jan 28 2022, 11:22 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
173

I'm not very familiar with these. Besides the memory access cost, are there any costs to take into account?

llvm/test/Analysis/CostModel/RISCV/masked_ldst.ll
28

Thanks, removed :p

LGTM but I would like to let @frasercrmck did final approve.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
173

That's pretty HW implementation dependent, so I guess this implementation is good enough for now, and let other RISC-V vendor to fill up their cost in future.

frasercrmck accepted this revision.Mar 1 2022, 7:52 AM

LGTM. As @kito-cheng said, this is a good starting point in case vendors want to start customizing things. Cheers. Sorry for the slow response.

This revision is now accepted and ready to land.Mar 1 2022, 7:52 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 6:48 AM
labath added a subscriber: labath.Mar 2 2022, 7:06 AM

This seems to fail on a couple of bots (e.g. https://lab.llvm.org/buildbot/#/builders/117/builds/5197), but you may not have gotten emails for those due to an unrelated breakage.

thakis added a subscriber: thakis.Mar 2 2022, 8:33 AM

Thanks for the revert!

alextsao1999 reopened this revision.Mar 2 2022, 10:16 AM

Sorry for that. Need to remove experimental.

This revision is now accepted and ready to land.Mar 2 2022, 10:16 AM
  • Remove experimental
This revision was landed with ongoing or failed builds.Mar 3 2022, 4:48 AM
This revision was automatically updated to reflect the committed changes.