>> So fusion in this context is really about identifying cases where two >> configuration settings are equivalent and you "fuse" them together. >> Presumably this is only going to be possible when the vector insns are >> just doing data movement rather than actual computations? >> If my understanding is correct, I can kind of see why you're doing >> fusion during phase 3. My sense is there's a better way, but I'm having >> a bit of trouble working out the details of what that should be to >> myself. In any event, revamping parts of the vsetvl insertion code >> isn't the kind of thing we should be doing now. The vsetvl demand fusion happens is not necessary "equivalent", instead, we call it we will do demand fusion when they are "compatible". And the fusion can happen between any vector insns including data movement and actual computations. What is "compatible" ?? This definition is according to RVV ISA. For example , For a vadd.vv need a vsetvl zero, 4, e32,m1,ta,ma. and a vle.v need a vsetvl zero,4,e8,mf4,ta,ma. According to RVV ISA: vadd.vv demand SEW = 32, LMUL = M1, AVL = 4 vle.v demand RATIO = SEW/LMUL = 32, AVL = 4. So after demand fusion, the demand becomes SEW = 32, LMUL = M1, AVL = 4. Such vsetvl instruction is configured as this demand fusion, we call it "compatible" since we can find a common vsetvl VL/VTYPE status for both vadd.vv and vle.v However, what case is not "incompatible", same example, if the vadd.vv demand SEW = 32. LMUL = MF2, the vadd.vv is incompatible with vle.v. since we can't find a common VL/VTYPE vsetvl instruction available for both of them. We have local demand fusion which is Phase 1. Local demand fusion is doing the fusion within a block And also we have global demand fusion which is Phase 3. Global demand fusion is doing across blocks. After Phase 1, each block has a single demand fusion. Phase 3 is doing global demand fusion trying to find the common VL/VTYPE status available for a bunch of blocks, and fuse them into a single vsetvl. So that we eliminate redundant vsetvli. Here is a example: bb 0: (vle.v demand RATIO = 32) / \ bb 1 bb 2 / \ / \ bb 3 bb 4 .... bb 5 vadd vmul vdiv (demand (demand (demand sew = 8, sew = 8, sew = 8, lmul = mf4) lmul = mf4, lmul = mf4, tail policy = tu) mask policy = mu) So in this case, we should do the global demand fusion for bb 0, bb3, bb 4, bb5. since they are compatible according to RVV ISA. The final demand info of vsetvl should be vsetvl e8,mf4,tu,mu and put it in the bb0. Then we can avoid vsetvl in bb 3, 4, 5. >> We have more fusion rules according to RVV ISA. Phase 3 (Global demand fusion) is >> really important. >> That would seem to indicate the function is poorly named. Unless you're >> using "empty" here to mean the state is valid or dirty. Either way it >> seems like the function name ought to be improved. >> The comments talk about bb1 being inside a loop. Nowhere do you check >> that as far as I can tell. >> When trying to understand what the patch is going I ran across this comment: >> /* The local_dem vector insn_info of the block. */ >> vector_insn_info local_dem; >> That comment really doesn't improve anything. "local_dem" is clearly >> short-hand for something (local demand?), whatever it is, make it >> clearer in the comment. Sorry for bad comments in the codes. Currently, I am working on the first patch of auto-vectorization. After I sent the first patch of auto-vectorization for you to review. I would like to re-check all the comments and code style of VSETVL PASS. And refine them. juzhe.zhong@rivai.ai From: Jeff Law Date: 2023-04-05 21:05 To: juzhe.zhong; gcc-patches CC: kito.cheng; palmer Subject: Re: [PATCH] RISC-V: Fix PR108279 On 4/2/23 16:40, juzhe.zhong@rivai.ai wrote: > This point is seletected not because LCM but by Phase 3 (VL/VTYPE demand > info backward fusion and propogation) which > is I introduced into VSETVL PASS to enhance LCM && improve vsetvl > instruction performance. So fusion in this context is really about identifying cases where two configuration settings are equivalent and you "fuse" them together. Presumably this is only going to be possible when the vector insns are just doing data movement rather than actual computations? If my understanding is correct, I can kind of see why you're doing fusion during phase 3. My sense is there's a better way, but I'm having a bit of trouble working out the details of what that should be to myself. In any event, revamping parts of the vsetvl insertion code isn't the kind of thing we should be doing now. WRT the actual patch. Please put a function comment on the all_empty_predecessor_p method. Something like this perhaps? /* Return TRUE if all the predecessors of CFG_BB have vsetvl state that is valid or dirty, FALSE otherwise. */ That would seem to indicate the function is poorly named. Unless you're using "empty" here to mean the state is valid or dirty. Either way it seems like the function name ought to be improved. The comments talk about bb1 being inside a loop. Nowhere do you check that as far as I can tell. When trying to understand what the patch is going I ran across this comment: /* The local_dem vector insn_info of the block. */ vector_insn_info local_dem; That comment really doesn't improve anything. "local_dem" is clearly short-hand for something (local demand?), whatever it is, make it clearer in the comment. Jeff