On Sat, 15 Feb 2025 08:47:16 PST (-0800), jeffreya...@gmail.com wrote:
On 2/13/25 3:54 PM, Edwin Lu wrote:The instruction scheduler appears to be speculatively hoisting vsetvl insns outside of their basic block without checking for data dependencies. This resulted in a situation where the following occurs vsetvli a5,a1,e32,m1,tu,ma vle32.v v2,0(a0) sub a1,a1,a5 <-- a1 potentially set to 0 sh2add a0,a5,a0 vfmacc.vv v1,v2,v2 vsetvli a5,a1,e32,m1,tu,ma <-- incompatible vinfo. update vl to 0 beq a1,zero,.L12 <-- check if avl is 0 This patch would essentially delay the vsetvl update to after the branch to prevent unnecessarily updating the vinfo at the end of a basic block. Since this is purely a performance related patch, gate the target hook with an opt flag to see the fallout. PR 117974 gcc/ChangeLog: * config/riscv/riscv.cc (riscv_sched_can_speculate_insn): (TARGET_SCHED_CAN_SPECULATE_INSN): Implement. * config/riscv/riscv.opt: Add temporary opt. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/vsetvl/pr117974.c: New test.I was kind of thinking we'd have a flag in the tuning structure so that each uarch could make a reasonable default selection rather than having it controlled by a flag.
Ya, seems reasonable. We wanted the flag to make it easier to do some expirements -- otherwise we'd need either two builds or an expirement-specific tune struct, and our perf guys aren't so great with SW so the flag is just easier.
I realize there may not be any designs upstream which would set this right now, but it puts the right infrastructure in place. Very similar to the divmod expansion stuff from a while back.
I'm fine either way. It's always kind of tough to tell what the right answer is on these things: we're taking complexity without a strong justification, but if it's an ISA corner case then it's not crazy to assume some HW will trip over it at some point. This one is so small I don't think it really matters which way we go, but if someone else is interested in using it that'd probably tip things towards getting it upstream on my end.
FWIW: This is just in the idea stage right now so it's no big deal if we don't merge it, it's both pretty easy to keep locally and of unknown utility (even internally). We've got to go look at the generated code either way and we'll post what we can publicly.
Edwin and I were also talking in the office yesterday and I think we can do a slightly better job here if we use the runqueue sorting hook. At that point we can actually look at the instructions and see if the branch will avoid a VL=0 case before preventing the hoisting, so we can avoid introducing unnecessary latency.
I put together some code that did what's in here via that sorting hook, but my attempts at matching more specific subtrees failed so I sent it to Edwin to try and figure out what I screwed up. It's probably something easy, I always screw that stuff up until I get it right.
So assuming that works we'll have another flavor of this to post.
Q. Does Rivos have a cpu defined in your internal tree? We do that for ours as keep it as a local change for now. Assuming you do, then adding the bits to the tuning structure should make it trivial to make this behavior the default on your design internally.
We've got internal tune structs. They're super easy to keep around and we'll likely always need them for internal stuff, so no big deal on that end.
