This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca][RISCV] Add RISCV-SEW instrument
ClosedPublic

Authored by michaelmaitland on Jun 29 2023, 12:59 PM.

Details

Summary

Now that RISCV pseudo instructions now account for SEW in some cases,
it useful that RISCV SEW instruments exist so that llvm-mca can use
the SEW specific scheduler classes.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 12:59 PM
michaelmaitland requested review of this revision.Jun 29 2023, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 12:59 PM

Fix error case tests

myhsu added inline comments.Jun 29 2023, 9:26 PM
llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h
46

maybe adding explicit?

llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s
8

is there any specific reason to test with the same LMUL here?

I've only left a minor comment. Otherwise, patch looks good to me.

llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp
135

I don't expect different instruments to have the same DESC_NAME. This should probably be an "else if".

michaelmaitland marked 2 inline comments as done.
  • Add explicit
  • change if -> else if
llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s
8

We need a LMUL instrument since the vdiv.vv instruction depends on LMUL and SEW. I chose to use the same LMUL to show that the MCA reports changed based on SEW changing.

The reason we need to provide an LMUL to test SEW is because vdiv.vv depends on both LMUL and SEW. We could not resolve the correct SchedClassID without both. I thought about having a LMUL-SEW instrument instead of a SEW instrument, but it becomes trickier to replace an active LMUL instrument with the LMUL from the LMUL-SEW instrument. It was simpler to just use two instruments. I am open to improving this in the future.

Last update reverted some tests by accident. This update un-reverts them.

This revision is now accepted and ready to land.Jun 30 2023, 7:16 AM

Represent SEW as bits<3> as per change in D154136

myhsu added inline comments.Jun 30 2023, 1:44 PM
llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s
8

the same LMUL to show that the MCA reports changed based on SEW changing.

Ahh that makes sense, thanks

We could not resolve the correct SchedClassID without both.

Does the current implementation effectively falls back to a default LMUL? If that's the case I think it's good enough as a baseline. That said, having a LMUL-SEW instrument would definitely be a nice addition.

llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s
8

It falls back to the “WorstCase” behavior. We have test coverage of no lmul nor sew provided, but I’ll add a test that gives coverage of lmul but no sew for an instruction that needs sew + lmul.

Add test case to verify when an instruction needs LMUL+SEW but only SEW provided that it falls back to WorstCase behavior

michaelmaitland marked an inline comment as done.Jun 30 2023, 2:00 PM
michaelmaitland added inline comments.
llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s
8

I've addressed this with llvm/test/tools/llvm-mca/RISCV/needs-sew-but-only-lmul.s

Add create instruments from vsetvli using SEW.

michaelmaitland planned changes to this revision.Jul 6 2023, 4:04 PM
michaelmaitland added inline comments.
llvm/test/tools/llvm-mca/RISCV/different-lmul-instruments.s
31

It looks like we're defaulting to worst case when SEW is provided, but the sched resource does not depend on SEW. This is occurring in many of the test cases. I need to fix.

Handle when SEW instrument exists, but Sched class depends only on LMUL.

This revision is now accepted and ready to land.Jul 6 2023, 4:07 PM
craig.topper accepted this revision.Jul 14 2023, 9:50 AM
This revision was landed with ongoing or failed builds.Jul 14 2023, 12:12 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/tools/llvm-mca/RISCV/needs-sew-but-only-lmul.s