Hi, Richard. Thanks for the comments. >> I think it would be worth saying in more detail why we only use SELECT_VL >> for single rgroups. I assume the reason is to simplify the pointer IV >> updates. Is that right?
Yes. >> The difficulty is >> that an rgroup that controls N vector loads (say) would need N pointer >> updates by variable amounts. But I'm not 100% sure whether we're >> avoiding that situation because it's difficult to code, or because >> it's inefficient. Or maybe we're avoiding it because it doesn't >> fit well with the later RVV vsetvl pass. I don't want to use SELECT_VL pattern for multiple rgroups since it's really ineffecient and also not fit well with latter RVV vsetvl PASS. (I have a draft in my downstream, the loop body becomes very ugly with a lot of instructions to adjust IVs). Since we define SELECT_VL as a flexible pattern that doesn't have the side effect set vector length, we need much more scalar operations to adjust the pointer IVs and it cause more vsetvli than just using MIN. Also, it changes a lot in middle-end and make middle-end codes too ugly and no benefits I see so far. Current approach (MIN), I think the current codegen is good (even though may not be perfect). Besides, LLVM only can handle one vector length (in GCC, we call multiple rgroup). I think RVV GCC is already in a good shape now in case of loop control. I'd rather support all RVV auto-vectorization features soon and focus on optimizing loopVectorizer (For example, I known GCC in TSVC has 46 fails, fixing failed vectorization case will improve much more, I think it can also improve ARM. ). Address comments and will send the next patch, really appreciate it. Thanks. juzhe.zh...@rivai.ai From: Richard Sandiford Date: 2023-06-05 05:59 To: juzhe.zhong CC: gcc-patches; rguenther Subject: Re: [PATCH] VECT: Add SELECT_VL support Sorry for the slow review. I don't know the IV-related parts well enough to review those properly, but they looked reasonable to me. Hopefully Richi can comment. I'm curious though. For: > + tree step = vect_dr_behavior (vinfo, dr_info)->step; > + > + [...] > + poly_uint64 bytesize = GET_MODE_SIZE (element_mode (aggr_type)); > + /* Since the outcome of .SELECT_VL is element size, we should adjust > + it into bytesize so that it can be used in address pointer variable > + amount IVs adjustment. */ > + tree tmp = fold_build2 (MULT_EXPR, len_type, loop_len, > + build_int_cst (len_type, bytesize)); > + if (tree_int_cst_sgn (step) == -1) > + tmp = fold_build1 (NEGATE_EXPR, len_type, tmp); Could you not just multiply loop_len by step, probably written as: build_int_cst (len_type, wi::to_widest (step)) avoiding the NEGATE_EXPR and bytesize calculation? step should represent the step of the original scalar IV, so doing that feels more direct. The loop-control bits look good to me apart from one hunk: > @@ -2737,6 +2738,14 @@ start_over: > LOOP_VINFO_VECT_FACTOR (loop_vinfo)))) > LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo) = true; > > + /* If we're using decrement IV and SELECT_VL is supported by the target. > + Use output of SELECT_VL to adjust IV of loop control and data reference. > + Note: We only use SELECT_VL on single-rgroup control. */ > + if (LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo) > + && LOOP_VINFO_LENS (loop_vinfo).length () == 1 > + && !slp) > + LOOP_VINFO_USING_SELECT_VL_P (loop_vinfo) = true; > + > /* If we're vectorizing an epilogue loop, the vectorized loop either needs > to be able to handle fewer than VF scalars, or needs to have a lower VF > than the main loop. */ This test also needs to check that the target implements the select_vl optab for the chosen iv type. You can check that using direct_internal_fn_supported_p. We should also check that LOOP_VINFO_LENS (loop_vinfo)[0].factor == 1, since the IV update multiplies by the size in bytes. I think it would be worth saying in more detail why we only use SELECT_VL for single rgroups. I assume the reason is to simplify the pointer IV updates. Is that right? That is: the multiple length controls that are currently generated from a MIN_EXPR IV should also work with a SELECT_VL IV. The difficulty is that an rgroup that controls N vector loads (say) would need N pointer updates by variable amounts. But I'm not 100% sure whether we're avoiding that situation because it's difficult to code, or because it's inefficient. Or maybe we're avoiding it because it doesn't fit well with the later RVV vsetvl pass. Thanks, Richard
