| Message ID | CABYV9SVTb_gcYDa-yeXi75tQg0yVkFsG3wUpS=L5VXH2BCArNw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > Hi > > Here is a patch to inform a programmer about the expanded vector operation. > Bootstrapped on x86-unknown-linux-gnu. > > ChangeLog: > > * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to > produce the warning. > (expand_vector_parallel): Adjust to produce the warning. Entries start without gcc/, they are relative to the gcc/ChangeLog file. > (lower_vec_shuffle): Adjust to produce the warning. > * gcc/common.opt: New warning Wvector-operation-expanded. > * gcc/doc/invoke.texi: Document the wawning. > > > Ok? I don't like the name -Wvector-operation-expanded. We emit a similar warning for missed inline expansions with -Winline, so maybe -Wvector-extensions (that's the name that appears in the C extension documentation). + location_t loc = gimple_location (gsi_stmt (*gsi)); + + warning_at (loc, OPT_Wvector_operation_expanded, + "vector operation will be expanded piecewise"); v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); for (i = 0; i < nunits; @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter tree result, compute_type; enum machine_mode mode; int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; + location_t loc = gimple_location (gsi_stmt (*gsi)); + + warning_at (loc, OPT_Wvector_operation_expanded, + "vector operation will be expanded in parallel"); what's the difference between 'piecewise' and 'in parallel'? @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter { int parts_per_word = UNITS_PER_WORD / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); + location_t loc = gimple_location (gsi_stmt (*gsi)); if (INTEGRAL_TYPE_P (TREE_TYPE (type)) && parts_per_word >= 4 && TYPE_VECTOR_SUBPARTS (type) >= 4) - return expand_vector_parallel (gsi, f_parallel, - type, a, b, code); + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); else - return expand_vector_piecewise (gsi, f, - type, TREE_TYPE (type), - a, b, code); + return expand_vector_piecewise (gsi, f, type, + TREE_TYPE (type), a, b, code); } /* Check if vector VEC consists of all the equal elements and unless i miss something loc is unused here. Please avoid random whitespace changes (just review your patch yourself before posting and revert pieces that do nothing). +@item -Wvector-operation-expanded +@opindex Wvector-operation-expanded +@opindex Wno-vector-operation-expanded +Warn if vector operation is not implemented via SIMD capabilities of the +architecture. Mainly useful for the performance tuning. I'd mention that this is for vector operations as of the C extension documented in "Vector Extensions". The vectorizer can produce some operations that will need further lowering - we probably should make sure to _not_ warn about those. Try running the vect.exp testsuite with the new warning turned on (eventually disabling SSE), like with obj/gcc> make check-gcc RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse vect.exp" > P.S. It is hard to write a reasonable testcase for the patch, because > one needs to guess which architecture would expand a given vector > operation. But the patch is trivial. You can create an aritificial large vector type for example, or put a testcase under gcc.target/i386 and disable SSE. We should have a testcase for this. Thanks, Richard.
On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> Hi >> >> Here is a patch to inform a programmer about the expanded vector operation. >> Bootstrapped on x86-unknown-linux-gnu. >> >> ChangeLog: >> >> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >> produce the warning. >> (expand_vector_parallel): Adjust to produce the warning. > > Entries start without gcc/, they are relative to the gcc/ChangeLog file. Sure, sorry. >> (lower_vec_shuffle): Adjust to produce the warning. >> * gcc/common.opt: New warning Wvector-operation-expanded. >> * gcc/doc/invoke.texi: Document the wawning. >> >> >> Ok? > > I don't like the name -Wvector-operation-expanded. We emit a > similar warning for missed inline expansions with -Winline, so > maybe -Wvector-extensions (that's the name that appears > in the C extension documentation). Hm, I don't care much about the name, unless it gets clear what the warning is used for. I am not really sure that Wvector-extensions makes it clear. Also, I don't see anything bad if the warning will pop up during the vectorisation. Any vector operation performed outside the SIMD accelerator looks suspicious, because it actually doesn't improve performance. Such a warning during the vectorisation could mean that a programmer forgot some flag, or the constant propagation failed to deliver a constant, or something else. Conceptually the text I am producing is not really a warning, it is more like an information, but I am not aware of the mechanisms that would allow me to introduce a flag triggering inform () or something similar. What I think we really need to avoid is including this warning in the standard Ox. > + location_t loc = gimple_location (gsi_stmt (*gsi)); > + > + warning_at (loc, OPT_Wvector_operation_expanded, > + "vector operation will be expanded piecewise"); > > v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); > for (i = 0; i < nunits; > @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter > tree result, compute_type; > enum machine_mode mode; > int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; > + location_t loc = gimple_location (gsi_stmt (*gsi)); > + > + warning_at (loc, OPT_Wvector_operation_expanded, > + "vector operation will be expanded in parallel"); > > what's the difference between 'piecewise' and 'in parallel'? Parallel is a little bit better for performance than piecewise. > @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter > { > int parts_per_word = UNITS_PER_WORD > / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); > + location_t loc = gimple_location (gsi_stmt (*gsi)); > > if (INTEGRAL_TYPE_P (TREE_TYPE (type)) > && parts_per_word >= 4 > && TYPE_VECTOR_SUBPARTS (type) >= 4) > - return expand_vector_parallel (gsi, f_parallel, > - type, a, b, code); > + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); > else > - return expand_vector_piecewise (gsi, f, > - type, TREE_TYPE (type), > - a, b, code); > + return expand_vector_piecewise (gsi, f, type, > + TREE_TYPE (type), a, b, code); > } > > /* Check if vector VEC consists of all the equal elements and > > unless i miss something loc is unused here. Please avoid random > whitespace changes (just review your patch yourself before posting > and revert pieces that do nothing). Yes you are right, sorry. > +@item -Wvector-operation-expanded > +@opindex Wvector-operation-expanded > +@opindex Wno-vector-operation-expanded > +Warn if vector operation is not implemented via SIMD capabilities of the > +architecture. Mainly useful for the performance tuning. > > I'd mention that this is for vector operations as of the C extension > documented in "Vector Extensions". > > The vectorizer can produce some operations that will need further > lowering - we probably should make sure to _not_ warn about those. > Try running the vect.exp testsuite with the new warning turned on > (eventually disabling SSE), like with > > obj/gcc> make check-gcc > RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse > vect.exp" Again, see the comment above. I think, if the warning can be triggered only manually, then we are fine. But I'll check anyway how many warnings I'll get from vect.exp. >> P.S. It is hard to write a reasonable testcase for the patch, because >> one needs to guess which architecture would expand a given vector >> operation. But the patch is trivial. > > You can create an aritificial large vector type for example, or put a > testcase under gcc.target/i386 and disable SSE. We should have > a testcase for this. Yeah, disabling SSE should help. Thanks, Artem. > Thanks, > Richard. >
On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >> <artyom.shinkaroff@gmail.com> wrote: >>> Hi >>> >>> Here is a patch to inform a programmer about the expanded vector operation. >>> Bootstrapped on x86-unknown-linux-gnu. >>> >>> ChangeLog: >>> >>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>> produce the warning. >>> (expand_vector_parallel): Adjust to produce the warning. >> >> Entries start without gcc/, they are relative to the gcc/ChangeLog file. > > Sure, sorry. > >>> (lower_vec_shuffle): Adjust to produce the warning. >>> * gcc/common.opt: New warning Wvector-operation-expanded. >>> * gcc/doc/invoke.texi: Document the wawning. >>> >>> >>> Ok? >> >> I don't like the name -Wvector-operation-expanded. We emit a >> similar warning for missed inline expansions with -Winline, so >> maybe -Wvector-extensions (that's the name that appears >> in the C extension documentation). > > Hm, I don't care much about the name, unless it gets clear what the > warning is used for. I am not really sure that Wvector-extensions > makes it clear. Also, I don't see anything bad if the warning will > pop up during the vectorisation. Any vector operation performed > outside the SIMD accelerator looks suspicious, because it actually > doesn't improve performance. Such a warning during the vectorisation > could mean that a programmer forgot some flag, or the constant > propagation failed to deliver a constant, or something else. > > Conceptually the text I am producing is not really a warning, it is > more like an information, but I am not aware of the mechanisms that > would allow me to introduce a flag triggering inform () or something > similar. > > What I think we really need to avoid is including this warning in the > standard Ox. > >> + location_t loc = gimple_location (gsi_stmt (*gsi)); >> + >> + warning_at (loc, OPT_Wvector_operation_expanded, >> + "vector operation will be expanded piecewise"); >> >> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >> for (i = 0; i < nunits; >> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >> tree result, compute_type; >> enum machine_mode mode; >> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >> + location_t loc = gimple_location (gsi_stmt (*gsi)); >> + >> + warning_at (loc, OPT_Wvector_operation_expanded, >> + "vector operation will be expanded in parallel"); >> >> what's the difference between 'piecewise' and 'in parallel'? > > Parallel is a little bit better for performance than piecewise. I see. That difference should probably be documented, maybe with an example. Richard. >> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >> { >> int parts_per_word = UNITS_PER_WORD >> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >> + location_t loc = gimple_location (gsi_stmt (*gsi)); >> >> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >> && parts_per_word >= 4 >> && TYPE_VECTOR_SUBPARTS (type) >= 4) >> - return expand_vector_parallel (gsi, f_parallel, >> - type, a, b, code); >> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >> else >> - return expand_vector_piecewise (gsi, f, >> - type, TREE_TYPE (type), >> - a, b, code); >> + return expand_vector_piecewise (gsi, f, type, >> + TREE_TYPE (type), a, b, code); >> } >> >> /* Check if vector VEC consists of all the equal elements and >> >> unless i miss something loc is unused here. Please avoid random >> whitespace changes (just review your patch yourself before posting >> and revert pieces that do nothing). > > Yes you are right, sorry. > >> +@item -Wvector-operation-expanded >> +@opindex Wvector-operation-expanded >> +@opindex Wno-vector-operation-expanded >> +Warn if vector operation is not implemented via SIMD capabilities of the >> +architecture. Mainly useful for the performance tuning. >> >> I'd mention that this is for vector operations as of the C extension >> documented in "Vector Extensions". >> >> The vectorizer can produce some operations that will need further >> lowering - we probably should make sure to _not_ warn about those. >> Try running the vect.exp testsuite with the new warning turned on >> (eventually disabling SSE), like with >> >> obj/gcc> make check-gcc >> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >> vect.exp" > > Again, see the comment above. I think, if the warning can be triggered > only manually, then we are fine. But I'll check anyway how many > warnings I'll get from vect.exp. > >>> P.S. It is hard to write a reasonable testcase for the patch, because >>> one needs to guess which architecture would expand a given vector >>> operation. But the patch is trivial. >> >> You can create an aritificial large vector type for example, or put a >> testcase under gcc.target/i386 and disable SSE. We should have >> a testcase for this. > > Yeah, disabling SSE should help. > > > Thanks, > Artem. >> Thanks, >> Richard. >> >
On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>> <artyom.shinkaroff@gmail.com> wrote: >>>> Hi >>>> >>>> Here is a patch to inform a programmer about the expanded vector operation. >>>> Bootstrapped on x86-unknown-linux-gnu. >>>> >>>> ChangeLog: >>>> >>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>> produce the warning. >>>> (expand_vector_parallel): Adjust to produce the warning. >>> >>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >> >> Sure, sorry. >> >>>> (lower_vec_shuffle): Adjust to produce the warning. >>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>> * gcc/doc/invoke.texi: Document the wawning. >>>> >>>> >>>> Ok? >>> >>> I don't like the name -Wvector-operation-expanded. We emit a >>> similar warning for missed inline expansions with -Winline, so >>> maybe -Wvector-extensions (that's the name that appears >>> in the C extension documentation). >> >> Hm, I don't care much about the name, unless it gets clear what the >> warning is used for. I am not really sure that Wvector-extensions >> makes it clear. Also, I don't see anything bad if the warning will >> pop up during the vectorisation. Any vector operation performed >> outside the SIMD accelerator looks suspicious, because it actually >> doesn't improve performance. Such a warning during the vectorisation >> could mean that a programmer forgot some flag, or the constant >> propagation failed to deliver a constant, or something else. >> >> Conceptually the text I am producing is not really a warning, it is >> more like an information, but I am not aware of the mechanisms that >> would allow me to introduce a flag triggering inform () or something >> similar. >> >> What I think we really need to avoid is including this warning in the >> standard Ox. >> >>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>> + >>> + warning_at (loc, OPT_Wvector_operation_expanded, >>> + "vector operation will be expanded piecewise"); >>> >>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>> for (i = 0; i < nunits; >>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>> tree result, compute_type; >>> enum machine_mode mode; >>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>> + >>> + warning_at (loc, OPT_Wvector_operation_expanded, >>> + "vector operation will be expanded in parallel"); >>> >>> what's the difference between 'piecewise' and 'in parallel'? >> >> Parallel is a little bit better for performance than piecewise. > > I see. That difference should probably be documented, maybe with > an example. > > Richard. > >>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>> { >>> int parts_per_word = UNITS_PER_WORD >>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>> >>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>> && parts_per_word >= 4 >>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>> - return expand_vector_parallel (gsi, f_parallel, >>> - type, a, b, code); >>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>> else >>> - return expand_vector_piecewise (gsi, f, >>> - type, TREE_TYPE (type), >>> - a, b, code); >>> + return expand_vector_piecewise (gsi, f, type, >>> + TREE_TYPE (type), a, b, code); >>> } >>> >>> /* Check if vector VEC consists of all the equal elements and >>> >>> unless i miss something loc is unused here. Please avoid random >>> whitespace changes (just review your patch yourself before posting >>> and revert pieces that do nothing). >> >> Yes you are right, sorry. >> >>> +@item -Wvector-operation-expanded >>> +@opindex Wvector-operation-expanded >>> +@opindex Wno-vector-operation-expanded >>> +Warn if vector operation is not implemented via SIMD capabilities of the >>> +architecture. Mainly useful for the performance tuning. >>> >>> I'd mention that this is for vector operations as of the C extension >>> documented in "Vector Extensions". >>> >>> The vectorizer can produce some operations that will need further >>> lowering - we probably should make sure to _not_ warn about those. >>> Try running the vect.exp testsuite with the new warning turned on >>> (eventually disabling SSE), like with >>> >>> obj/gcc> make check-gcc >>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>> vect.exp" >> >> Again, see the comment above. I think, if the warning can be triggered >> only manually, then we are fine. But I'll check anyway how many >> warnings I'll get from vect.exp. >> >>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>> one needs to guess which architecture would expand a given vector >>>> operation. But the patch is trivial. >>> >>> You can create an aritificial large vector type for example, or put a >>> testcase under gcc.target/i386 and disable SSE. We should have >>> a testcase for this. >> >> Yeah, disabling SSE should help. >> >> >> Thanks, >> Artem. >>> Thanks, >>> Richard. >>> >> > New version of the patch in the attachment with the test-cases. Bootstrapped on x86_64-apple-darwin10.8.0. Currently is being tested. Richard, I've checked the vect.exp case, as you suggested. It caused a lot of failures, but not because of the new warning. The main reason is -mno-sse. The target is capable to vectorize, so the dg option expects tests to pass, but the artificial option makes them fail. Checking the new warning on vect.exp without -mno-sse, it didn't cause any new failures. Anyway, we should be pretty much safe, cause the warning is not a part of -O3. Thanks, Artem.
On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >> <artyom.shinkaroff@gmail.com> wrote: >>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>> <artyom.shinkaroff@gmail.com> wrote: >>>>> Hi >>>>> >>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>> >>>>> ChangeLog: >>>>> >>>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>> produce the warning. >>>>> (expand_vector_parallel): Adjust to produce the warning. >>>> >>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>> >>> Sure, sorry. >>> >>>>> (lower_vec_shuffle): Adjust to produce the warning. >>>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>>> * gcc/doc/invoke.texi: Document the wawning. >>>>> >>>>> >>>>> Ok? >>>> >>>> I don't like the name -Wvector-operation-expanded. We emit a >>>> similar warning for missed inline expansions with -Winline, so >>>> maybe -Wvector-extensions (that's the name that appears >>>> in the C extension documentation). >>> >>> Hm, I don't care much about the name, unless it gets clear what the >>> warning is used for. I am not really sure that Wvector-extensions >>> makes it clear. Also, I don't see anything bad if the warning will >>> pop up during the vectorisation. Any vector operation performed >>> outside the SIMD accelerator looks suspicious, because it actually >>> doesn't improve performance. Such a warning during the vectorisation >>> could mean that a programmer forgot some flag, or the constant >>> propagation failed to deliver a constant, or something else. >>> >>> Conceptually the text I am producing is not really a warning, it is >>> more like an information, but I am not aware of the mechanisms that >>> would allow me to introduce a flag triggering inform () or something >>> similar. >>> >>> What I think we really need to avoid is including this warning in the >>> standard Ox. >>> >>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>> + >>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>> + "vector operation will be expanded piecewise"); >>>> >>>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>> for (i = 0; i < nunits; >>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>> tree result, compute_type; >>>> enum machine_mode mode; >>>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>> + >>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>> + "vector operation will be expanded in parallel"); >>>> >>>> what's the difference between 'piecewise' and 'in parallel'? >>> >>> Parallel is a little bit better for performance than piecewise. >> >> I see. That difference should probably be documented, maybe with >> an example. >> >> Richard. >> >>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>> { >>>> int parts_per_word = UNITS_PER_WORD >>>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>> >>>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>> && parts_per_word >= 4 >>>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>> - return expand_vector_parallel (gsi, f_parallel, >>>> - type, a, b, code); >>>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>> else >>>> - return expand_vector_piecewise (gsi, f, >>>> - type, TREE_TYPE (type), >>>> - a, b, code); >>>> + return expand_vector_piecewise (gsi, f, type, >>>> + TREE_TYPE (type), a, b, code); >>>> } >>>> >>>> /* Check if vector VEC consists of all the equal elements and >>>> >>>> unless i miss something loc is unused here. Please avoid random >>>> whitespace changes (just review your patch yourself before posting >>>> and revert pieces that do nothing). >>> >>> Yes you are right, sorry. >>> >>>> +@item -Wvector-operation-expanded >>>> +@opindex Wvector-operation-expanded >>>> +@opindex Wno-vector-operation-expanded >>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>> +architecture. Mainly useful for the performance tuning. >>>> >>>> I'd mention that this is for vector operations as of the C extension >>>> documented in "Vector Extensions". >>>> >>>> The vectorizer can produce some operations that will need further >>>> lowering - we probably should make sure to _not_ warn about those. >>>> Try running the vect.exp testsuite with the new warning turned on >>>> (eventually disabling SSE), like with >>>> >>>> obj/gcc> make check-gcc >>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>> vect.exp" >>> >>> Again, see the comment above. I think, if the warning can be triggered >>> only manually, then we are fine. But I'll check anyway how many >>> warnings I'll get from vect.exp. >>> >>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>> one needs to guess which architecture would expand a given vector >>>>> operation. But the patch is trivial. >>>> >>>> You can create an aritificial large vector type for example, or put a >>>> testcase under gcc.target/i386 and disable SSE. We should have >>>> a testcase for this. >>> >>> Yeah, disabling SSE should help. >>> >>> >>> Thanks, >>> Artem. >>>> Thanks, >>>> Richard. >>>> >>> >> > > New version of the patch in the attachment with the test-cases. > Bootstrapped on x86_64-apple-darwin10.8.0. > Currently is being tested. > > > Richard, I've checked the vect.exp case, as you suggested. It caused > a lot of failures, but not because of the new warning. The main > reason is -mno-sse. The target is capable to vectorize, so the dg > option expects tests to pass, but the artificial option makes them > fail. Checking the new warning on vect.exp without -mno-sse, it > didn't cause any new failures. Anyway, we should be pretty much safe, > cause the warning is not a part of -O3. > > Thanks, > Artem. > Successfully regression-tested on x86_64-apple-darwin10.8.0. ChangeLog: gcc/ * doc/invoke.texi: Document new warning. * common.opt (Wvector-operation-performance): Define new warning. * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded vector operation. (exapnd_vector_parallel): Warn about expanded vector operation. (lower_vec_shuffle): Warn about expanded vector operation. * c-parser.c (c_parser_postfix_expression): Assign correct location when creating VEC_SHUFFLE_EXPR. gcc/testsuite/ * gcc.target/i386/warn-vect-op-3.c: New test. * gcc.target/i386/warn-vect-op-1.c: New test. * gcc.target/i386/warn-vect-op-2.c: New test. Ok for trunk? Thanks, Artem.
On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >>> <artyom.shinkaroff@gmail.com> wrote: >>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>>> <richard.guenther@gmail.com> wrote: >>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>> Hi >>>>>> >>>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>> >>>>>> ChangeLog: >>>>>> >>>>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>>> produce the warning. >>>>>> (expand_vector_parallel): Adjust to produce the warning. >>>>> >>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>>> >>>> Sure, sorry. >>>> >>>>>> (lower_vec_shuffle): Adjust to produce the warning. >>>>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>>>> * gcc/doc/invoke.texi: Document the wawning. >>>>>> >>>>>> >>>>>> Ok? >>>>> >>>>> I don't like the name -Wvector-operation-expanded. We emit a >>>>> similar warning for missed inline expansions with -Winline, so >>>>> maybe -Wvector-extensions (that's the name that appears >>>>> in the C extension documentation). >>>> >>>> Hm, I don't care much about the name, unless it gets clear what the >>>> warning is used for. I am not really sure that Wvector-extensions >>>> makes it clear. Also, I don't see anything bad if the warning will >>>> pop up during the vectorisation. Any vector operation performed >>>> outside the SIMD accelerator looks suspicious, because it actually >>>> doesn't improve performance. Such a warning during the vectorisation >>>> could mean that a programmer forgot some flag, or the constant >>>> propagation failed to deliver a constant, or something else. >>>> >>>> Conceptually the text I am producing is not really a warning, it is >>>> more like an information, but I am not aware of the mechanisms that >>>> would allow me to introduce a flag triggering inform () or something >>>> similar. >>>> >>>> What I think we really need to avoid is including this warning in the >>>> standard Ox. >>>> >>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>> + >>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>> + "vector operation will be expanded piecewise"); >>>>> >>>>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>>> for (i = 0; i < nunits; >>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>> tree result, compute_type; >>>>> enum machine_mode mode; >>>>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>> + >>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>> + "vector operation will be expanded in parallel"); >>>>> >>>>> what's the difference between 'piecewise' and 'in parallel'? >>>> >>>> Parallel is a little bit better for performance than piecewise. >>> >>> I see. That difference should probably be documented, maybe with >>> an example. >>> >>> Richard. >>> >>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>> { >>>>> int parts_per_word = UNITS_PER_WORD >>>>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>> >>>>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>> && parts_per_word >= 4 >>>>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>>> - return expand_vector_parallel (gsi, f_parallel, >>>>> - type, a, b, code); >>>>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>>> else >>>>> - return expand_vector_piecewise (gsi, f, >>>>> - type, TREE_TYPE (type), >>>>> - a, b, code); >>>>> + return expand_vector_piecewise (gsi, f, type, >>>>> + TREE_TYPE (type), a, b, code); >>>>> } >>>>> >>>>> /* Check if vector VEC consists of all the equal elements and >>>>> >>>>> unless i miss something loc is unused here. Please avoid random >>>>> whitespace changes (just review your patch yourself before posting >>>>> and revert pieces that do nothing). >>>> >>>> Yes you are right, sorry. >>>> >>>>> +@item -Wvector-operation-expanded >>>>> +@opindex Wvector-operation-expanded >>>>> +@opindex Wno-vector-operation-expanded >>>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>>> +architecture. Mainly useful for the performance tuning. >>>>> >>>>> I'd mention that this is for vector operations as of the C extension >>>>> documented in "Vector Extensions". >>>>> >>>>> The vectorizer can produce some operations that will need further >>>>> lowering - we probably should make sure to _not_ warn about those. >>>>> Try running the vect.exp testsuite with the new warning turned on >>>>> (eventually disabling SSE), like with >>>>> >>>>> obj/gcc> make check-gcc >>>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>>> vect.exp" >>>> >>>> Again, see the comment above. I think, if the warning can be triggered >>>> only manually, then we are fine. But I'll check anyway how many >>>> warnings I'll get from vect.exp. >>>> >>>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>>> one needs to guess which architecture would expand a given vector >>>>>> operation. But the patch is trivial. >>>>> >>>>> You can create an aritificial large vector type for example, or put a >>>>> testcase under gcc.target/i386 and disable SSE. We should have >>>>> a testcase for this. >>>> >>>> Yeah, disabling SSE should help. >>>> >>>> >>>> Thanks, >>>> Artem. >>>>> Thanks, >>>>> Richard. >>>>> >>>> >>> >> >> New version of the patch in the attachment with the test-cases. >> Bootstrapped on x86_64-apple-darwin10.8.0. >> Currently is being tested. >> >> >> Richard, I've checked the vect.exp case, as you suggested. It caused >> a lot of failures, but not because of the new warning. The main >> reason is -mno-sse. The target is capable to vectorize, so the dg >> option expects tests to pass, but the artificial option makes them >> fail. Checking the new warning on vect.exp without -mno-sse, it >> didn't cause any new failures. Anyway, we should be pretty much safe, >> cause the warning is not a part of -O3. >> >> Thanks, >> Artem. >> > > Successfully regression-tested on x86_64-apple-darwin10.8.0. > > ChangeLog: > gcc/ > * doc/invoke.texi: Document new warning. > * common.opt (Wvector-operation-performance): Define new warning. > * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded > vector operation. > (exapnd_vector_parallel): Warn about expanded vector operation. > (lower_vec_shuffle): Warn about expanded vector operation. > * c-parser.c (c_parser_postfix_expression): Assign correct location > when creating VEC_SHUFFLE_EXPR. > > gcc/testsuite/ > * gcc.target/i386/warn-vect-op-3.c: New test. > * gcc.target/i386/warn-vect-op-1.c: New test. > * gcc.target/i386/warn-vect-op-2.c: New test. > > Ok for trunk? + if (gimple_expr_type (gsi_stmt (*gsi)) == type) + warning_at (loc, OPT_Wvector_operation_performance, + "vector operation will be expanded piecewise"); + else + warning_at (loc, OPT_Wvector_operation_performance, + "vector operation will be expanded in parallel"); we should not check for exact type equivalence here. Please use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) instead. We could also consider to pass down the kind of lowering from the caller (or warn in the callers). @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0); compute_type = lang_hooks.types.type_for_mode (mode, 1); result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code); + warning_at (loc, OPT_Wvector_operation_performance, + "vector operation will be expanded with a " + "single scalar operation"); That means it will be fast, no? Why warn for it at all? @@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter return expand_vector_parallel (gsi, f_parallel, type, a, b, code); else - return expand_vector_piecewise (gsi, f, + return expand_vector_piecewise (gsi, f, type, TREE_TYPE (type), a, b, code); } You add trailing space here ... (please review your patches yourself for this kind of errors) + { + expr.value = + c_build_vec_shuffle_expr + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, + NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); + SET_EXPR_LOCATION (expr.value, loc); That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr. If then that routine needs fixing. Thanks, Richard. > > Thanks, > Artem. >
On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov >> <artyom.shinkaroff@gmail.com> wrote: >>> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >>>> <artyom.shinkaroff@gmail.com> wrote: >>>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>>>> <richard.guenther@gmail.com> wrote: >>>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>>> Hi >>>>>>> >>>>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>>> >>>>>>> ChangeLog: >>>>>>> >>>>>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>>>> produce the warning. >>>>>>> (expand_vector_parallel): Adjust to produce the warning. >>>>>> >>>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>>>> >>>>> Sure, sorry. >>>>> >>>>>>> (lower_vec_shuffle): Adjust to produce the warning. >>>>>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>>>>> * gcc/doc/invoke.texi: Document the wawning. >>>>>>> >>>>>>> >>>>>>> Ok? >>>>>> >>>>>> I don't like the name -Wvector-operation-expanded. We emit a >>>>>> similar warning for missed inline expansions with -Winline, so >>>>>> maybe -Wvector-extensions (that's the name that appears >>>>>> in the C extension documentation). >>>>> >>>>> Hm, I don't care much about the name, unless it gets clear what the >>>>> warning is used for. I am not really sure that Wvector-extensions >>>>> makes it clear. Also, I don't see anything bad if the warning will >>>>> pop up during the vectorisation. Any vector operation performed >>>>> outside the SIMD accelerator looks suspicious, because it actually >>>>> doesn't improve performance. Such a warning during the vectorisation >>>>> could mean that a programmer forgot some flag, or the constant >>>>> propagation failed to deliver a constant, or something else. >>>>> >>>>> Conceptually the text I am producing is not really a warning, it is >>>>> more like an information, but I am not aware of the mechanisms that >>>>> would allow me to introduce a flag triggering inform () or something >>>>> similar. >>>>> >>>>> What I think we really need to avoid is including this warning in the >>>>> standard Ox. >>>>> >>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> + >>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>> + "vector operation will be expanded piecewise"); >>>>>> >>>>>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>>>> for (i = 0; i < nunits; >>>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>>> tree result, compute_type; >>>>>> enum machine_mode mode; >>>>>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> + >>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>> + "vector operation will be expanded in parallel"); >>>>>> >>>>>> what's the difference between 'piecewise' and 'in parallel'? >>>>> >>>>> Parallel is a little bit better for performance than piecewise. >>>> >>>> I see. That difference should probably be documented, maybe with >>>> an example. >>>> >>>> Richard. >>>> >>>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>>> { >>>>>> int parts_per_word = UNITS_PER_WORD >>>>>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> >>>>>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>>> && parts_per_word >= 4 >>>>>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>>>> - return expand_vector_parallel (gsi, f_parallel, >>>>>> - type, a, b, code); >>>>>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>>>> else >>>>>> - return expand_vector_piecewise (gsi, f, >>>>>> - type, TREE_TYPE (type), >>>>>> - a, b, code); >>>>>> + return expand_vector_piecewise (gsi, f, type, >>>>>> + TREE_TYPE (type), a, b, code); >>>>>> } >>>>>> >>>>>> /* Check if vector VEC consists of all the equal elements and >>>>>> >>>>>> unless i miss something loc is unused here. Please avoid random >>>>>> whitespace changes (just review your patch yourself before posting >>>>>> and revert pieces that do nothing). >>>>> >>>>> Yes you are right, sorry. >>>>> >>>>>> +@item -Wvector-operation-expanded >>>>>> +@opindex Wvector-operation-expanded >>>>>> +@opindex Wno-vector-operation-expanded >>>>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>>>> +architecture. Mainly useful for the performance tuning. >>>>>> >>>>>> I'd mention that this is for vector operations as of the C extension >>>>>> documented in "Vector Extensions". >>>>>> >>>>>> The vectorizer can produce some operations that will need further >>>>>> lowering - we probably should make sure to _not_ warn about those. >>>>>> Try running the vect.exp testsuite with the new warning turned on >>>>>> (eventually disabling SSE), like with >>>>>> >>>>>> obj/gcc> make check-gcc >>>>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>>>> vect.exp" >>>>> >>>>> Again, see the comment above. I think, if the warning can be triggered >>>>> only manually, then we are fine. But I'll check anyway how many >>>>> warnings I'll get from vect.exp. >>>>> >>>>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>>>> one needs to guess which architecture would expand a given vector >>>>>>> operation. But the patch is trivial. >>>>>> >>>>>> You can create an aritificial large vector type for example, or put a >>>>>> testcase under gcc.target/i386 and disable SSE. We should have >>>>>> a testcase for this. >>>>> >>>>> Yeah, disabling SSE should help. >>>>> >>>>> >>>>> Thanks, >>>>> Artem. >>>>>> Thanks, >>>>>> Richard. >>>>>> >>>>> >>>> >>> >>> New version of the patch in the attachment with the test-cases. >>> Bootstrapped on x86_64-apple-darwin10.8.0. >>> Currently is being tested. >>> >>> >>> Richard, I've checked the vect.exp case, as you suggested. It caused >>> a lot of failures, but not because of the new warning. The main >>> reason is -mno-sse. The target is capable to vectorize, so the dg >>> option expects tests to pass, but the artificial option makes them >>> fail. Checking the new warning on vect.exp without -mno-sse, it >>> didn't cause any new failures. Anyway, we should be pretty much safe, >>> cause the warning is not a part of -O3. >>> >>> Thanks, >>> Artem. >>> >> >> Successfully regression-tested on x86_64-apple-darwin10.8.0. >> >> ChangeLog: >> gcc/ >> * doc/invoke.texi: Document new warning. >> * common.opt (Wvector-operation-performance): Define new warning. >> * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded >> vector operation. >> (exapnd_vector_parallel): Warn about expanded vector operation. >> (lower_vec_shuffle): Warn about expanded vector operation. >> * c-parser.c (c_parser_postfix_expression): Assign correct location >> when creating VEC_SHUFFLE_EXPR. >> >> gcc/testsuite/ >> * gcc.target/i386/warn-vect-op-3.c: New test. >> * gcc.target/i386/warn-vect-op-1.c: New test. >> * gcc.target/i386/warn-vect-op-2.c: New test. >> >> Ok for trunk? > > + if (gimple_expr_type (gsi_stmt (*gsi)) == type) > + warning_at (loc, OPT_Wvector_operation_performance, > + "vector operation will be expanded piecewise"); > + else > + warning_at (loc, OPT_Wvector_operation_performance, > + "vector operation will be expanded in parallel"); > > we should not check for exact type equivalence here. Please > use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) > instead. We could also consider to pass down the kind of lowering > from the caller (or warn in the callers). Ok, Fixed. > > @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter > mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0); > compute_type = lang_hooks.types.type_for_mode (mode, 1); > result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code); > + warning_at (loc, OPT_Wvector_operation_performance, > + "vector operation will be expanded with a " > + "single scalar operation"); > > That means it will be fast, no? Why warn for it at all? Most likely it means sower. Eventually it is a different kind of the expansion. This situation could happen when you work with MMX vectors, and by some reason instead of a single MMX operation, you will have register operation + masking. > > @@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter > return expand_vector_parallel (gsi, f_parallel, > type, a, b, code); > else > - return expand_vector_piecewise (gsi, f, > + return expand_vector_piecewise (gsi, f, > type, TREE_TYPE (type), > a, b, code); > } > > You add trailing space here ... (please review your patches yourself > for this kind of errors) > > + { > + expr.value = > + c_build_vec_shuffle_expr > + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, > + NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); > + SET_EXPR_LOCATION (expr.value, loc); > > That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr. > If then that routine needs fixing. Ok, moved to c-typeck.c. The new version is in the attachment. Boostrapped on x86_64-apple-darwin10.8.0. Ok? Thanks, Artem. > Thanks, > Richard. > >> >> Thanks, >> Artem. >> >
On Mon, Oct 10, 2011 at 3:21 PM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov >> <artyom.shinkaroff@gmail.com> wrote: >>> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov >>> <artyom.shinkaroff@gmail.com> wrote: >>>> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther >>>> <richard.guenther@gmail.com> wrote: >>>>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>>>>> <richard.guenther@gmail.com> wrote: >>>>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>>>> Hi >>>>>>>> >>>>>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>>>> >>>>>>>> ChangeLog: >>>>>>>> >>>>>>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>>>>> produce the warning. >>>>>>>> (expand_vector_parallel): Adjust to produce the warning. >>>>>>> >>>>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>>>>> >>>>>> Sure, sorry. >>>>>> >>>>>>>> (lower_vec_shuffle): Adjust to produce the warning. >>>>>>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>>>>>> * gcc/doc/invoke.texi: Document the wawning. >>>>>>>> >>>>>>>> >>>>>>>> Ok? >>>>>>> >>>>>>> I don't like the name -Wvector-operation-expanded. We emit a >>>>>>> similar warning for missed inline expansions with -Winline, so >>>>>>> maybe -Wvector-extensions (that's the name that appears >>>>>>> in the C extension documentation). >>>>>> >>>>>> Hm, I don't care much about the name, unless it gets clear what the >>>>>> warning is used for. I am not really sure that Wvector-extensions >>>>>> makes it clear. Also, I don't see anything bad if the warning will >>>>>> pop up during the vectorisation. Any vector operation performed >>>>>> outside the SIMD accelerator looks suspicious, because it actually >>>>>> doesn't improve performance. Such a warning during the vectorisation >>>>>> could mean that a programmer forgot some flag, or the constant >>>>>> propagation failed to deliver a constant, or something else. >>>>>> >>>>>> Conceptually the text I am producing is not really a warning, it is >>>>>> more like an information, but I am not aware of the mechanisms that >>>>>> would allow me to introduce a flag triggering inform () or something >>>>>> similar. >>>>>> >>>>>> What I think we really need to avoid is including this warning in the >>>>>> standard Ox. >>>>>> >>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>> + >>>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>>> + "vector operation will be expanded piecewise"); >>>>>>> >>>>>>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>>>>> for (i = 0; i < nunits; >>>>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>>>> tree result, compute_type; >>>>>>> enum machine_mode mode; >>>>>>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>> + >>>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>>> + "vector operation will be expanded in parallel"); >>>>>>> >>>>>>> what's the difference between 'piecewise' and 'in parallel'? >>>>>> >>>>>> Parallel is a little bit better for performance than piecewise. >>>>> >>>>> I see. That difference should probably be documented, maybe with >>>>> an example. >>>>> >>>>> Richard. >>>>> >>>>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>>>> { >>>>>>> int parts_per_word = UNITS_PER_WORD >>>>>>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>> >>>>>>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>>>> && parts_per_word >= 4 >>>>>>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>>>>> - return expand_vector_parallel (gsi, f_parallel, >>>>>>> - type, a, b, code); >>>>>>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>>>>> else >>>>>>> - return expand_vector_piecewise (gsi, f, >>>>>>> - type, TREE_TYPE (type), >>>>>>> - a, b, code); >>>>>>> + return expand_vector_piecewise (gsi, f, type, >>>>>>> + TREE_TYPE (type), a, b, code); >>>>>>> } >>>>>>> >>>>>>> /* Check if vector VEC consists of all the equal elements and >>>>>>> >>>>>>> unless i miss something loc is unused here. Please avoid random >>>>>>> whitespace changes (just review your patch yourself before posting >>>>>>> and revert pieces that do nothing). >>>>>> >>>>>> Yes you are right, sorry. >>>>>> >>>>>>> +@item -Wvector-operation-expanded >>>>>>> +@opindex Wvector-operation-expanded >>>>>>> +@opindex Wno-vector-operation-expanded >>>>>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>>>>> +architecture. Mainly useful for the performance tuning. >>>>>>> >>>>>>> I'd mention that this is for vector operations as of the C extension >>>>>>> documented in "Vector Extensions". >>>>>>> >>>>>>> The vectorizer can produce some operations that will need further >>>>>>> lowering - we probably should make sure to _not_ warn about those. >>>>>>> Try running the vect.exp testsuite with the new warning turned on >>>>>>> (eventually disabling SSE), like with >>>>>>> >>>>>>> obj/gcc> make check-gcc >>>>>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>>>>> vect.exp" >>>>>> >>>>>> Again, see the comment above. I think, if the warning can be triggered >>>>>> only manually, then we are fine. But I'll check anyway how many >>>>>> warnings I'll get from vect.exp. >>>>>> >>>>>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>>>>> one needs to guess which architecture would expand a given vector >>>>>>>> operation. But the patch is trivial. >>>>>>> >>>>>>> You can create an aritificial large vector type for example, or put a >>>>>>> testcase under gcc.target/i386 and disable SSE. We should have >>>>>>> a testcase for this. >>>>>> >>>>>> Yeah, disabling SSE should help. >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Artem. >>>>>>> Thanks, >>>>>>> Richard. >>>>>>> >>>>>> >>>>> >>>> >>>> New version of the patch in the attachment with the test-cases. >>>> Bootstrapped on x86_64-apple-darwin10.8.0. >>>> Currently is being tested. >>>> >>>> >>>> Richard, I've checked the vect.exp case, as you suggested. It caused >>>> a lot of failures, but not because of the new warning. The main >>>> reason is -mno-sse. The target is capable to vectorize, so the dg >>>> option expects tests to pass, but the artificial option makes them >>>> fail. Checking the new warning on vect.exp without -mno-sse, it >>>> didn't cause any new failures. Anyway, we should be pretty much safe, >>>> cause the warning is not a part of -O3. >>>> >>>> Thanks, >>>> Artem. >>>> >>> >>> Successfully regression-tested on x86_64-apple-darwin10.8.0. >>> >>> ChangeLog: >>> gcc/ >>> * doc/invoke.texi: Document new warning. >>> * common.opt (Wvector-operation-performance): Define new warning. >>> * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded >>> vector operation. >>> (exapnd_vector_parallel): Warn about expanded vector operation. >>> (lower_vec_shuffle): Warn about expanded vector operation. >>> * c-parser.c (c_parser_postfix_expression): Assign correct location >>> when creating VEC_SHUFFLE_EXPR. >>> >>> gcc/testsuite/ >>> * gcc.target/i386/warn-vect-op-3.c: New test. >>> * gcc.target/i386/warn-vect-op-1.c: New test. >>> * gcc.target/i386/warn-vect-op-2.c: New test. >>> >>> Ok for trunk? >> >> + if (gimple_expr_type (gsi_stmt (*gsi)) == type) >> + warning_at (loc, OPT_Wvector_operation_performance, >> + "vector operation will be expanded piecewise"); >> + else >> + warning_at (loc, OPT_Wvector_operation_performance, >> + "vector operation will be expanded in parallel"); >> >> we should not check for exact type equivalence here. Please >> use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) >> instead. We could also consider to pass down the kind of lowering >> from the caller (or warn in the callers). > > Ok, Fixed. >> >> @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter >> mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0); >> compute_type = lang_hooks.types.type_for_mode (mode, 1); >> result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code); >> + warning_at (loc, OPT_Wvector_operation_performance, >> + "vector operation will be expanded with a " >> + "single scalar operation"); >> >> That means it will be fast, no? Why warn for it at all? > > Most likely it means sower. Eventually it is a different kind of the > expansion. This situation could happen when you work with MMX > vectors, and by some reason instead of a single MMX operation, you > will have register operation + masking. >> >> @@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter >> return expand_vector_parallel (gsi, f_parallel, >> type, a, b, code); >> else >> - return expand_vector_piecewise (gsi, f, >> + return expand_vector_piecewise (gsi, f, >> type, TREE_TYPE (type), >> a, b, code); >> } >> >> You add trailing space here ... (please review your patches yourself >> for this kind of errors) >> >> + { >> + expr.value = >> + c_build_vec_shuffle_expr >> + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, >> + NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); >> + SET_EXPR_LOCATION (expr.value, loc); >> >> That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr. >> If then that routine needs fixing. > > Ok, moved to c-typeck.c. > > > The new version is in the attachment. Boostrapped on x86_64-apple-darwin10.8.0. > Ok? Ok with @@ -2934,7 +2934,8 @@ c_build_vec_perm_expr (location_t loc, t if (!wrap) ret = c_wrap_maybe_const (ret, true); - + + SET_EXPR_LOCATION (ret, loc); return ret; instead of this use build3_loc (loc, ...) when building the VEC_PERM_EXPR in the line before this hunk. Thanks, Richard. > > Thanks, > Artem. > > >> Thanks, >> Richard. >> >>> >>> Thanks, >>> Artem. >>> >> >
On Tue, Oct 11, 2011 at 11:52 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Mon, Oct 10, 2011 at 3:21 PM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov >>> <artyom.shinkaroff@gmail.com> wrote: >>>> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov >>>> <artyom.shinkaroff@gmail.com> wrote: >>>>> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther >>>>> <richard.guenther@gmail.com> wrote: >>>>>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >>>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>>>>>> <richard.guenther@gmail.com> wrote: >>>>>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>>>>>> <artyom.shinkaroff@gmail.com> wrote: >>>>>>>>> Hi >>>>>>>>> >>>>>>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>>>>> >>>>>>>>> ChangeLog: >>>>>>>>> >>>>>>>>> * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>>>>>> produce the warning. >>>>>>>>> (expand_vector_parallel): Adjust to produce the warning. >>>>>>>> >>>>>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>>>>>> >>>>>>> Sure, sorry. >>>>>>> >>>>>>>>> (lower_vec_shuffle): Adjust to produce the warning. >>>>>>>>> * gcc/common.opt: New warning Wvector-operation-expanded. >>>>>>>>> * gcc/doc/invoke.texi: Document the wawning. >>>>>>>>> >>>>>>>>> >>>>>>>>> Ok? >>>>>>>> >>>>>>>> I don't like the name -Wvector-operation-expanded. We emit a >>>>>>>> similar warning for missed inline expansions with -Winline, so >>>>>>>> maybe -Wvector-extensions (that's the name that appears >>>>>>>> in the C extension documentation). >>>>>>> >>>>>>> Hm, I don't care much about the name, unless it gets clear what the >>>>>>> warning is used for. I am not really sure that Wvector-extensions >>>>>>> makes it clear. Also, I don't see anything bad if the warning will >>>>>>> pop up during the vectorisation. Any vector operation performed >>>>>>> outside the SIMD accelerator looks suspicious, because it actually >>>>>>> doesn't improve performance. Such a warning during the vectorisation >>>>>>> could mean that a programmer forgot some flag, or the constant >>>>>>> propagation failed to deliver a constant, or something else. >>>>>>> >>>>>>> Conceptually the text I am producing is not really a warning, it is >>>>>>> more like an information, but I am not aware of the mechanisms that >>>>>>> would allow me to introduce a flag triggering inform () or something >>>>>>> similar. >>>>>>> >>>>>>> What I think we really need to avoid is including this warning in the >>>>>>> standard Ox. >>>>>>> >>>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>>> + >>>>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>>>> + "vector operation will be expanded piecewise"); >>>>>>>> >>>>>>>> v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>>>>>> for (i = 0; i < nunits; >>>>>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>>>>> tree result, compute_type; >>>>>>>> enum machine_mode mode; >>>>>>>> int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>>> + >>>>>>>> + warning_at (loc, OPT_Wvector_operation_expanded, >>>>>>>> + "vector operation will be expanded in parallel"); >>>>>>>> >>>>>>>> what's the difference between 'piecewise' and 'in parallel'? >>>>>>> >>>>>>> Parallel is a little bit better for performance than piecewise. >>>>>> >>>>>> I see. That difference should probably be documented, maybe with >>>>>> an example. >>>>>> >>>>>> Richard. >>>>>> >>>>>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>>>>> { >>>>>>>> int parts_per_word = UNITS_PER_WORD >>>>>>>> / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>>>>>> + location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>>>> >>>>>>>> if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>>>>> && parts_per_word >= 4 >>>>>>>> && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>>>>>> - return expand_vector_parallel (gsi, f_parallel, >>>>>>>> - type, a, b, code); >>>>>>>> + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>>>>>> else >>>>>>>> - return expand_vector_piecewise (gsi, f, >>>>>>>> - type, TREE_TYPE (type), >>>>>>>> - a, b, code); >>>>>>>> + return expand_vector_piecewise (gsi, f, type, >>>>>>>> + TREE_TYPE (type), a, b, code); >>>>>>>> } >>>>>>>> >>>>>>>> /* Check if vector VEC consists of all the equal elements and >>>>>>>> >>>>>>>> unless i miss something loc is unused here. Please avoid random >>>>>>>> whitespace changes (just review your patch yourself before posting >>>>>>>> and revert pieces that do nothing). >>>>>>> >>>>>>> Yes you are right, sorry. >>>>>>> >>>>>>>> +@item -Wvector-operation-expanded >>>>>>>> +@opindex Wvector-operation-expanded >>>>>>>> +@opindex Wno-vector-operation-expanded >>>>>>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>>>>>> +architecture. Mainly useful for the performance tuning. >>>>>>>> >>>>>>>> I'd mention that this is for vector operations as of the C extension >>>>>>>> documented in "Vector Extensions". >>>>>>>> >>>>>>>> The vectorizer can produce some operations that will need further >>>>>>>> lowering - we probably should make sure to _not_ warn about those. >>>>>>>> Try running the vect.exp testsuite with the new warning turned on >>>>>>>> (eventually disabling SSE), like with >>>>>>>> >>>>>>>> obj/gcc> make check-gcc >>>>>>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>>>>>> vect.exp" >>>>>>> >>>>>>> Again, see the comment above. I think, if the warning can be triggered >>>>>>> only manually, then we are fine. But I'll check anyway how many >>>>>>> warnings I'll get from vect.exp. >>>>>>> >>>>>>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>>>>>> one needs to guess which architecture would expand a given vector >>>>>>>>> operation. But the patch is trivial. >>>>>>>> >>>>>>>> You can create an aritificial large vector type for example, or put a >>>>>>>> testcase under gcc.target/i386 and disable SSE. We should have >>>>>>>> a testcase for this. >>>>>>> >>>>>>> Yeah, disabling SSE should help. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Artem. >>>>>>>> Thanks, >>>>>>>> Richard. >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> New version of the patch in the attachment with the test-cases. >>>>> Bootstrapped on x86_64-apple-darwin10.8.0. >>>>> Currently is being tested. >>>>> >>>>> >>>>> Richard, I've checked the vect.exp case, as you suggested. It caused >>>>> a lot of failures, but not because of the new warning. The main >>>>> reason is -mno-sse. The target is capable to vectorize, so the dg >>>>> option expects tests to pass, but the artificial option makes them >>>>> fail. Checking the new warning on vect.exp without -mno-sse, it >>>>> didn't cause any new failures. Anyway, we should be pretty much safe, >>>>> cause the warning is not a part of -O3. >>>>> >>>>> Thanks, >>>>> Artem. >>>>> >>>> >>>> Successfully regression-tested on x86_64-apple-darwin10.8.0. >>>> >>>> ChangeLog: >>>> gcc/ >>>> * doc/invoke.texi: Document new warning. >>>> * common.opt (Wvector-operation-performance): Define new warning. >>>> * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded >>>> vector operation. >>>> (exapnd_vector_parallel): Warn about expanded vector operation. >>>> (lower_vec_shuffle): Warn about expanded vector operation. >>>> * c-parser.c (c_parser_postfix_expression): Assign correct location >>>> when creating VEC_SHUFFLE_EXPR. >>>> >>>> gcc/testsuite/ >>>> * gcc.target/i386/warn-vect-op-3.c: New test. >>>> * gcc.target/i386/warn-vect-op-1.c: New test. >>>> * gcc.target/i386/warn-vect-op-2.c: New test. >>>> >>>> Ok for trunk? >>> >>> + if (gimple_expr_type (gsi_stmt (*gsi)) == type) >>> + warning_at (loc, OPT_Wvector_operation_performance, >>> + "vector operation will be expanded piecewise"); >>> + else >>> + warning_at (loc, OPT_Wvector_operation_performance, >>> + "vector operation will be expanded in parallel"); >>> >>> we should not check for exact type equivalence here. Please >>> use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) >>> instead. We could also consider to pass down the kind of lowering >>> from the caller (or warn in the callers). >> >> Ok, Fixed. >>> >>> @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter >>> mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0); >>> compute_type = lang_hooks.types.type_for_mode (mode, 1); >>> result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code); >>> + warning_at (loc, OPT_Wvector_operation_performance, >>> + "vector operation will be expanded with a " >>> + "single scalar operation"); >>> >>> That means it will be fast, no? Why warn for it at all? >> >> Most likely it means sower. Eventually it is a different kind of the >> expansion. This situation could happen when you work with MMX >> vectors, and by some reason instead of a single MMX operation, you >> will have register operation + masking. >>> >>> @@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter >>> return expand_vector_parallel (gsi, f_parallel, >>> type, a, b, code); >>> else >>> - return expand_vector_piecewise (gsi, f, >>> + return expand_vector_piecewise (gsi, f, >>> type, TREE_TYPE (type), >>> a, b, code); >>> } >>> >>> You add trailing space here ... (please review your patches yourself >>> for this kind of errors) >>> >>> + { >>> + expr.value = >>> + c_build_vec_shuffle_expr >>> + (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, >>> + NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); >>> + SET_EXPR_LOCATION (expr.value, loc); >>> >>> That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr. >>> If then that routine needs fixing. >> >> Ok, moved to c-typeck.c. >> >> >> The new version is in the attachment. Boostrapped on x86_64-apple-darwin10.8.0. >> Ok? > > Ok with > > @@ -2934,7 +2934,8 @@ c_build_vec_perm_expr (location_t loc, t > > if (!wrap) > ret = c_wrap_maybe_const (ret, true); > - > + > + SET_EXPR_LOCATION (ret, loc); > return ret; > > instead of this use build3_loc (loc, ...) when building the VEC_PERM_EXPR > in the line before this hunk. > > Thanks, > Richard. > >> >> Thanks, >> Artem. >> >> >>> Thanks, >>> Richard. >>> >>>> >>>> Thanks, >>>> Artem. >>>> >>> >> > Committed with the revision 179807. Artem.
On Tue, Oct 11, 2011 at 9:11 AM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > > Committed with the revision 179807. > > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704
This patch fixed PR50704.
gcc/testsuite:
* gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target.
* gcc.target/i386/warn-vect-op-1.c: Ditto.
* gcc.target/i386/warn-vect-op-2.c: Ditto.
Ok for trunk?
Artem.
On Wed, Oct 12, 2011 at 4:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 11, 2011 at 9:11 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>>
>> Committed with the revision 179807.
>>
>>
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704
>
> --
> H.J.
>
On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: > This patch fixed PR50704. > > gcc/testsuite: > * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. > * gcc.target/i386/warn-vect-op-1.c: Ditto. > * gcc.target/i386/warn-vect-op-2.c: Ditto. > > Ok for trunk? Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling.
On Thu, Oct 13, 2011 at 10:59 AM, Mike Stump <mikestump@comcast.net> wrote: > On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: >> This patch fixed PR50704. >> >> gcc/testsuite: >> * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. >> * gcc.target/i386/warn-vect-op-1.c: Ditto. >> * gcc.target/i386/warn-vect-op-2.c: Ditto. >> >> Ok for trunk? > > Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling. I suppose you instead want sth like { dg-require-effective-target lp64 } ?
On Thu, Oct 13, 2011 at 10:23 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, Oct 13, 2011 at 10:59 AM, Mike Stump <mikestump@comcast.net> wrote: >> On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: >>> This patch fixed PR50704. >>> >>> gcc/testsuite: >>> * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. >>> * gcc.target/i386/warn-vect-op-1.c: Ditto. >>> * gcc.target/i386/warn-vect-op-2.c: Ditto. >>> >>> Ok for trunk? >> >> Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling. > > I suppose you instead want sth like > > { dg-require-effective-target lp64 } > > ? > See our discussion with HJ here: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704 /* { dg-do compile { target { ! { ia32 } } } } */ was his idea. As far as x32 sets UNITS_PER_WORD to 8, these tests should work fine. Artem.
On Thu, Oct 13, 2011 at 10:40 AM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Thu, Oct 13, 2011 at 10:23 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Thu, Oct 13, 2011 at 10:59 AM, Mike Stump <mikestump@comcast.net> wrote: >>> On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: >>>> This patch fixed PR50704. >>>> >>>> gcc/testsuite: >>>> * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. >>>> * gcc.target/i386/warn-vect-op-1.c: Ditto. >>>> * gcc.target/i386/warn-vect-op-2.c: Ditto. >>>> >>>> Ok for trunk? >>> >>> Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling. >> >> I suppose you instead want sth like >> >> { dg-require-effective-target lp64 } >> >> ? >> > > See our discussion with HJ here: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704 > /* { dg-do compile { target { ! { ia32 } } } } */ was his idea. As > far as x32 sets UNITS_PER_WORD to 8, these tests should work fine. > > Artem. > Ping. So can I commit the changes? Thanks, Artem.
On Fri, Oct 14, 2011 at 3:42 PM, Artem Shinkarov <artyom.shinkaroff@gmail.com> wrote: > On Thu, Oct 13, 2011 at 10:40 AM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Thu, Oct 13, 2011 at 10:23 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Thu, Oct 13, 2011 at 10:59 AM, Mike Stump <mikestump@comcast.net> wrote: >>>> On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: >>>>> This patch fixed PR50704. >>>>> >>>>> gcc/testsuite: >>>>> * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. >>>>> * gcc.target/i386/warn-vect-op-1.c: Ditto. >>>>> * gcc.target/i386/warn-vect-op-2.c: Ditto. >>>>> >>>>> Ok for trunk? >>>> >>>> Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling. >>> >>> I suppose you instead want sth like >>> >>> { dg-require-effective-target lp64 } >>> >>> ? >>> >> >> See our discussion with HJ here: >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704 >> /* { dg-do compile { target { ! { ia32 } } } } */ was his idea. As >> far as x32 sets UNITS_PER_WORD to 8, these tests should work fine. >> >> Artem. >> > > Ping. > > So can I commit the changes? Yes. Thanks, Richard. > > Thanks, > Artem. >
On Fri, Oct 14, 2011 at 2:57 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Fri, Oct 14, 2011 at 3:42 PM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> On Thu, Oct 13, 2011 at 10:40 AM, Artem Shinkarov >> <artyom.shinkaroff@gmail.com> wrote: >>> On Thu, Oct 13, 2011 at 10:23 AM, Richard Guenther >>> <richard.guenther@gmail.com> wrote: >>>> On Thu, Oct 13, 2011 at 10:59 AM, Mike Stump <mikestump@comcast.net> wrote: >>>>> On Oct 12, 2011, at 2:37 PM, Artem Shinkarov wrote: >>>>>> This patch fixed PR50704. >>>>>> >>>>>> gcc/testsuite: >>>>>> * gcc.target/i386/warn-vect-op-3.c: Exclude ia32 target. >>>>>> * gcc.target/i386/warn-vect-op-1.c: Ditto. >>>>>> * gcc.target/i386/warn-vect-op-2.c: Ditto. >>>>>> >>>>>> Ok for trunk? >>>>> >>>>> Ok. Is this x32 clean? :-) If not, HJ will offer an even better spelling. >>>> >>>> I suppose you instead want sth like >>>> >>>> { dg-require-effective-target lp64 } >>>> >>>> ? >>>> >>> >>> See our discussion with HJ here: >>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50704 >>> /* { dg-do compile { target { ! { ia32 } } } } */ was his idea. As >>> far as x32 sets UNITS_PER_WORD to 8, these tests should work fine. >>> >>> Artem. >>> >> >> Ping. >> >> So can I commit the changes? > > Yes. > > Thanks, > Richard. Committed with 179991. >> >> Thanks, >> Artem. >> >
On Oct 14, 2011, at 8:37 AM, Artem Shinkarov wrote:
> Committed with 179991.
Please don't send these... If you commit for a person, you can send directly to them the fact you committed the work. If people want to know when works goes in, be sure to use a PR and put yourself on the cc list, then, you will get the email it was committed from the version control system, after it hits spinning disk. Thanks.
Index: gcc/tree-vect-generic.c =================================================================== --- gcc/tree-vect-generic.c (revision 179464) +++ gcc/tree-vect-generic.c (working copy) @@ -235,6 +235,10 @@ expand_vector_piecewise (gimple_stmt_ite int delta = tree_low_cst (part_width, 1) / tree_low_cst (TYPE_SIZE (TREE_TYPE (type)), 1); int i; + location_t loc = gimple_location (gsi_stmt (*gsi)); + + warning_at (loc, OPT_Wvector_operation_expanded, + "vector operation will be expanded piecewise"); v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); for (i = 0; i < nunits; @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter tree result, compute_type; enum machine_mode mode; int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; + location_t loc = gimple_location (gsi_stmt (*gsi)); + + warning_at (loc, OPT_Wvector_operation_expanded, + "vector operation will be expanded in parallel"); /* We have three strategies. If the type is already correct, just do the operation an element at a time. Else, if the vector is wider than @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter { int parts_per_word = UNITS_PER_WORD / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); + location_t loc = gimple_location (gsi_stmt (*gsi)); if (INTEGRAL_TYPE_P (TREE_TYPE (type)) && parts_per_word >= 4 && TYPE_VECTOR_SUBPARTS (type) >= 4) - return expand_vector_parallel (gsi, f_parallel, - type, a, b, code); + return expand_vector_parallel (gsi, f_parallel, type, a, b, code); else - return expand_vector_piecewise (gsi, f, - type, TREE_TYPE (type), - a, b, code); + return expand_vector_piecewise (gsi, f, type, + TREE_TYPE (type), a, b, code); } /* Check if vector VEC consists of all the equal elements and @@ -400,8 +407,8 @@ expand_vector_operation (gimple_stmt_ite case PLUS_EXPR: case MINUS_EXPR: if (!TYPE_OVERFLOW_TRAPS (type)) - return expand_vector_addition (gsi, do_binop, do_plus_minus, type, - gimple_assign_rhs1 (assign), + return expand_vector_addition (gsi, do_binop, do_plus_minus, type, + gimple_assign_rhs1 (assign), gimple_assign_rhs2 (assign), code); break; @@ -622,6 +629,8 @@ lower_vec_shuffle (gimple_stmt_iterator return true; } + warning_at (loc, OPT_Wvector_operation_expanded, + "vector shuffling operation will be expanded piecewise"); if (operand_equal_p (vec0, vec1, 0)) { unsigned i; Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 179464) +++ gcc/common.opt (working copy) @@ -694,6 +694,10 @@ Wcoverage-mismatch Common Var(warn_coverage_mismatch) Init(1) Warning Warn in case profiles in -fprofile-use do not match +Wvector-operation-expanded +Common Var(warn_vector_operation_expanded) Warning +Warn when a vector operation is expanded piecewise + Xassembler Driver Separate Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 179464) +++ gcc/doc/invoke.texi (working copy) @@ -271,7 +271,8 @@ Objective-C and Objective-C++ Dialects}. -Wunused-label -Wunused-local-typedefs -Wunused-parameter @gol -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol --Wvariadic-macros -Wvla -Wvolatile-register-var -Wwrite-strings} +-Wvariadic-macros -Wvector-operation-expanded -Wvla +-Wvolatile-register-var -Wwrite-strings} @item C and Objective-C-only Warning Options @gccoptlist{-Wbad-function-cast -Wmissing-declarations @gol @@ -4532,6 +4533,12 @@ Warn if variadic macros are used in peda alternate syntax when in pedantic ISO C99 mode. This is default. To inhibit the warning messages, use @option{-Wno-variadic-macros}. +@item -Wvector-operation-expanded +@opindex Wvector-operation-expanded +@opindex Wno-vector-operation-expanded +Warn if vector operation is not implemented via SIMD capabilities of the +architecture. Mainly useful for the performance tuning. + @item -Wvla @opindex Wvla @opindex Wno-vla
Hi Here is a patch to inform a programmer about the expanded vector operation. Bootstrapped on x86-unknown-linux-gnu. ChangeLog: * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to produce the warning. (expand_vector_parallel): Adjust to produce the warning. (lower_vec_shuffle): Adjust to produce the warning. * gcc/common.opt: New warning Wvector-operation-expanded. * gcc/doc/invoke.texi: Document the wawning. Ok? Thanks, Artem Shinkarov. P.S. It is hard to write a reasonable testcase for the patch, because one needs to guess which architecture would expand a given vector operation. But the patch is trivial.