[PATCH] D154142: [llvm-mca][RISCV] Add RISCV-SEW instrument

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland marked an inline comment as done.
michaelmaitland added inline comments.


================
Comment at: llvm/test/tools/llvm-mca/RISCV/different-sew-instruments.s:8
+vdiv.vv v8, v8, v12
+vsetvli zero, a0, e64, m1, tu, mu
+# LLVM-MCA-RISCV-SEW E64
----------------
michaelmaitland wrote:
> myhsu wrote:
> > michaelmaitland wrote:
> > > myhsu wrote:
> > > > is there any specific reason to test with the same LMUL here?
> > > 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.
> > >  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.
> 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. 
I've addressed this with llvm/test/tools/llvm-mca/RISCV/needs-sew-but-only-lmul.s 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154142/new/

https://reviews.llvm.org/D154142