On Thu, Nov 8, 2018 at 12:02 PM Renlin Li <renlin...@foss.arm.com> wrote: > > Hi all, > > When allow-store-data-races is enabled, ifcvt would prefer to generated > conditional select and unconditional store to convert certain if statement > into: > > _ifc_1 = val > _ifc_2 = A[i] > val = cond? _ifc_1 : _ifc_2 > A[i] = val > > When the loop got vectorized, this pattern will be turned into > MASK_LOAD, VEC_COND_EXPR and MASK_STORE. This could be improved.
I'm somewhat confused - the vectorizer doesn't generate a masked store when if-conversion didn't create one in the first place. In particular with allow-store-data-races=1 (what your testcase uses) there are no masked loads/stores generated at all. So at least you need a better testcase to motivate (one that doesn't load from array[i] so that we know the conditional stores might trap). So what I see (with store data races not allowed) from ifcvt is <bb 3> [local count: 1006632961]: # i_20 = PHI <i_13(9), 1(21)> # ivtmp_18 = PHI <ivtmp_5(9), 15(21)> a_10 = array[i_20]; _1 = a_10 & 1; _2 = a_10 + 1; _32 = _1 != 0; _33 = &array[i_20]; .MASK_STORE (_33, 32B, _32, _2); prephitmp_8 = _1 != 0 ? _2 : a_10; _4 = a_10 + 2; _34 = prephitmp_8 > 10; .MASK_STORE (_33, 32B, _34, _4); i_13 = i_20 + 1; ivtmp_5 = ivtmp_18 - 1; if (ivtmp_5 != 0) and what you want to do is merge prephitmp_8 = _1 != 0 ? _2 : a_10; _34 = prephitmp_8 > 10; somehow? But your patch mentions that _4 should be prephitmp_8 so it wouldn't do anything here? > The change here add a post processing function to combine the VEC_COND_EXPR > expression into MASK_STORE, and delete related dead code. > > I am a little bit conservative here. > I didn't change the default behavior of ifcvt to always generate MASK_STORE, > although it should be safe in all cases (allow or dis-allow store data race). > > MASK_STORE might not well handled in vectorization pass compared with > conditional select. It might be too early and aggressive to do that in ifcvt. > And the performance of MASK_STORE might not good for some platforms. > (We could add --param or target hook to differentiate this ifcvt behavior > on different platforms) > > Another reason I did not do that in ifcvt is the data reference > analysis. To create a MASK_STORE, a pointer is created as the first > argument to the internal function call. If the pointer is created out of > array references, e.g. x = &A[i], data reference analysis could not properly > analysis the relationship between MEM_REF (x) and ARRAY_REF (A, i). This > will create a versioned loop beside the vectorized one. Actually for your testcase it won't vectorize because there's compile-time aliasing (somehow we miss handling of dependence distance zero?!) > (I have hacks to look through the MEM_REF, and restore the reference back to > ARRAY_REF (A, i). Maybe we could do analysis on lowered address expression? > I saw we have gimple laddress pass to aid the vectorizer) An old idea of mine is to have dependence analysis fall back to lowered address form when it fails to match two references. This would require re-analysis and eventually storing an alternate "inner reference" in the data-ref structure. > The approach here comes a little bit late, on the condition that vector > MASK_STORE is generated by loop vectorizer. In this case, it is definitely > beneficial to do the code transformation. > > Any thoughts on the best way to fix the issue? > > > This patch has been tested with aarch64-none-elf, no regressions. > > Regards, > Renlin > > gcc/ChangeLog: > > 2018-11-08 Renlin Li <renlin...@arm.com> > > * tree-vectorizer.h (combine_sel_mask_store): Declare new function. > * tree-vect-loop.c (combine_sel_mask_store): Define new function. > * tree-vectorizer.c (vectorize_loops): Call it. > > gcc/testsuite/ChangeLog: > > 2018-11-08 Renlin Li <renlin...@arm.com> > > * gcc.target/aarch64/sve/combine_vcond_mask_store_1.c: New. >
