On Mon, Sep 14, 2020 at 10:05 AM luoxhu <luo...@linux.ibm.com> wrote: > > > > On 2020/9/10 18:08, Richard Biener wrote: > > On Wed, Sep 9, 2020 at 6:03 PM Segher Boessenkool > > <seg...@kernel.crashing.org> wrote: > >> > >> On Wed, Sep 09, 2020 at 04:28:19PM +0200, Richard Biener wrote: > >>> On Wed, Sep 9, 2020 at 3:49 PM Segher Boessenkool > >>> <seg...@kernel.crashing.org> wrote: > >>>> > >>>> Hi! > >>>> > >>>> On Tue, Sep 08, 2020 at 10:26:51AM +0200, Richard Biener wrote: > >>>>> Hmm, yeah - I guess that's what should be addressed first then. > >>>>> I'm quite sure that in case 'v' is not on the stack but in memory like > >>>>> in my case a SImode store is better than what we get from > >>>>> vec_insert - in fact vec_insert will likely introduce a RMW cycle > >>>>> which is prone to inserting store-data-races? > >>>> > >>>> The other way around -- if it is in memory, and was stored as vector > >>>> recently, then reading back something shorter from it is prone to > >>>> SHL/LHS problems. There is nothing special about the stack here, except > >>>> of course it is more likely to have been stored recently if on the > >>>> stack. So it depends how often it has been stored recently which option > >>>> is best. On newer CPUs, although they can avoid SHL/LHS flushes more > >>>> often, the penalty is relatively bigger, so memory does not often win. > >>>> > >>>> I.e.: it needs to be measured. Intuition is often wrong here. > >>> > >>> But the current method would simply do a direct store to memory > >>> without a preceeding read of the whole vector. > >> > >> The problem is even worse the other way: you do a short store here, but > >> so a full vector read later. If the store and read are far apart, that > >> is fine, but if they are close (that is on the order of fifty or more > >> insns), there can be problems. > > > > Sure, but you can't simply load/store a whole vector when the code didn't > > unless you know it will not introduce data races and it will not trap (thus > > the whole vector needs to be at least naturally aligned). > > > > Also if there's a preceeding short store you will now load the whole vector > > to avoid the short store ... catch-22 > > > >> There often are problems over function calls (where the compiler cannot > >> usually *see* how something is used). > > > > Yep. The best way would be to use small loads and larger stores > > which is what CPUs usually tend to handle fine (with alignment > > constraints, etc.). Of course that's not what either of the "solutions" > > can do. > > > > That said, since you seem to be "first" in having an instruction > > to insert into a vector at a variable position the idea that we'd > > have to spill anyway for this to be expanded and thus we expand > > the vector to a stack location in the first place falls down. And > > that's where I'd first try to improve things. > > > > So what can the CPU actually do? > > > > Not sure whether this reflects the issues you discussed above. > I constructed below test cases and tested with and without this patch, > only if "a+c"(which means store only), the performance is getting bad with > this patch due to extra load/store(about 50% slower). > For "v = vec_insert (i, v, n);" usage, v is always loaded after store, so this > patch will always get big benefit. > But for "v[n % 4] = i;" usage, it depends on whether "v" is used immediately > inside the function or out of the function soon. Does this mean unify the two > usage to same gimple code not a good idea sometimes? Or is it possible to > detect the generated IFN ".VEC_INSERT (&v, i_4(D), _1);" destination "v" is > used not far away inside or out side of the function? > > > #define TYPE int > > vector TYPE v = {1, 2, 3, 4}; // a. global vector. > vector TYPE s = {4, 3, 2, 1}; > > __attribute__ ((noinline)) > vector TYPE > test (vector TYPE u, TYPE i, size_t n) > { > v[n % 4] = i;
^^^ this should be u[n % 4] = i; I guess. Is the % 4 mandated by the vec_insert semantics btw? If you tested with the above error you probably need to re-measure. On gimple the above function (after fixing it) looks like VIEW_CONVERT_EXPR<int[4]>(u)[_1] = i_4(D); and the IFN idea I had would - for non-global memory 'u' only - transform this to vector_register_2 = u; vector_register_3 = .IFN_VEC_SET (vector_register_2, _1, i_4(D)); u = vector_register_3; if vec_set can handle variable indexes. This then becomes a vec_set on a register and if that was the only variable indexing of 'u' will also cause 'u' to be expanded as register rather than stack memory. Note we can't use the direct-optab method here since the vec_set optab modifies operand 0 which isn't possible in SSA form. That might hint at that we eventually want to extend BIT_INSERT_EXPR to handle a non-constant bit position but for experiments using an alternate internal function is certainly easier. Richard. > return u; > } > > int main () > { > //vector TYPE v = {1, 2, 3, 4}; // b. local vector. > //vector TYPE s = {4, 3, 2, 1}; > vector TYPE r = {0}; > for (long k = 0; k < 3989478945; k++) > { > r += test (s, 254.0f, k); // c. access different vector. store only. > //r += test (v, 254.0f, k); // d. access same vector. load after > store in callee function. > //test (s, 254.0f, k); r += v; //e. load after store out of function. > } > return r[0]; > } > > > Thanks, > Xionghu >