diff mbox

OMP_CLAUSES with clauses in operand 0

Message ID 877fsvrxuu.fsf@schwinge.name
State New
Headers show

Commit Message

Thomas Schwinge April 29, 2015, 11:13 a.m. UTC 
Hi!

On Wed, 29 Apr 2015 11:32:31 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> Yeah, it is a non-starter, it has unnecessary runtime overhead everywhere
> where it is used.

Huh.  OMP_CLAUSES is currently used in a dozen places in
cp/cp-gimplify.c, cp/pt.c, and gimplify.c.  I've been expecting my
changed code to be well-optimizable, for the compiler already knows
TREE_CODE(NODE) in a lot of these places.  Doing a quick before (1)/after
(2) comparison test with -g -O2 on x86_64 GNU/Linux using GCC 4.8.3, the
object file sizes are as follows:

       text    data     bss     dec     hex filename
      37027       0      16   37043    90b3 1/build-gcc/gcc/cp/cp-gimplify.o
      36307       0      16   36323    8de3 2/build-gcc/gcc/cp/cp-gimplify.o
       text    data     bss     dec     hex filename
     458742       0     136  458878   7007e 1/build-gcc/gcc/cp/pt.o
     458630       0     136  458766   7000e 2/build-gcc/gcc/cp/pt.o
       text    data     bss     dec     hex filename
     166056       0      48  166104   288d8 1/build-gcc/gcc/gimplify.o
     166200       0      48  166248   28968 2/build-gcc/gcc/gimplify.o

..., so actually smaller for the first two files.  Admittedly, there is a
144 bytes (0.0867 %) size increase in gimplify.o, and I have not measured
the actual runtime overhead (which I'm totally expecting to be "in the
noise", but...), so I'm assuming that my proposal to keep the interface
simple does not pay for the presumed runtime overhead, so I guess I'm
posting this patch just for the archives...


If that's not been obvious, I favor code readability (one generic
OMP_CLAUSES interface instead of having both OMP_CLAUSES and
OMP_CLAUSES_STANDALONE) over a tiny code size regression, but you seem to
think differently, set a different policy for code changes, which I'll
have to yield to.


Grüße,
 Thomas

Comments

Jakub Jelinek April 29, 2015, 11:43 a.m. UTC | #1 
On Wed, Apr 29, 2015 at 01:13:29PM +0200, Thomas Schwinge wrote:
> On Wed, 29 Apr 2015 11:32:31 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> > Yeah, it is a non-starter, it has unnecessary runtime overhead everywhere
> > where it is used.
> 
> Huh.  OMP_CLAUSES is currently used in a dozen places in
> cp/cp-gimplify.c, cp/pt.c, and gimplify.c.  I've been expecting my
> changed code to be well-optimizable, for the compiler already knows
> TREE_CODE(NODE) in a lot of these places.  Doing a quick before (1)/after
> (2) comparison test with -g -O2 on x86_64 GNU/Linux using GCC 4.8.3, the
> object file sizes are as follows:
> 
>        text    data     bss     dec     hex filename
>       37027       0      16   37043    90b3 1/build-gcc/gcc/cp/cp-gimplify.o
>       36307       0      16   36323    8de3 2/build-gcc/gcc/cp/cp-gimplify.o
>        text    data     bss     dec     hex filename
>      458742       0     136  458878   7007e 1/build-gcc/gcc/cp/pt.o
>      458630       0     136  458766   7000e 2/build-gcc/gcc/cp/pt.o
>        text    data     bss     dec     hex filename
>      166056       0      48  166104   288d8 1/build-gcc/gcc/gimplify.o
>      166200       0      48  166248   28968 2/build-gcc/gcc/gimplify.o
> 
> ..., so actually smaller for the first two files.  Admittedly, there is a
> 144 bytes (0.0867 %) size increase in gimplify.o, and I have not measured
> the actual runtime overhead (which I'm totally expecting to be "in the
> noise", but...), so I'm assuming that my proposal to keep the interface
> simple does not pay for the presumed runtime overhead, so I guess I'm
> posting this patch just for the archives...

I really don't understand how you could get smaller code out of that, that
doesn't make any sense.  And I don't see where it would make sense to share
the same code for handling the standalone directives and directives with
associated blocks, in all the places I've looked at you want a different
code.  And in many cases you don't have just a single TREE_CODE, but
multiple, likely all of them from the same category (either all of them
smaller/equal or all larger than OMP_SINGLE), but VRP doesn't have to be
able to fix up the weirdnesses.

So yes, I really prefer OMP_STANDALONE_CLAUSES over OMP_CLAUSES for
everything.  And, tree checking which is on by default should catch this
properly, it was just a matter of tests not being written when they should
have been.

	Jakub
diff mbox

Patch

--- gcc/tree.h
+++ gcc/tree.h
@@ -1200,7 +1200,9 @@  extern void protected_set_expr_location (tree, location_t);
 #define OMP_BODY(NODE) \
   TREE_OPERAND (TREE_RANGE_CHECK (NODE, OACC_PARALLEL, OMP_CRITICAL), 0)
 #define OMP_CLAUSES(NODE) \
-  TREE_OPERAND (TREE_RANGE_CHECK (NODE, OACC_PARALLEL, OMP_SINGLE), 1)
+  ((TREE_CODE (NODE) <= OMP_SINGLE) \
+   ? TREE_OPERAND (TREE_RANGE_CHECK (NODE, OACC_PARALLEL, OMP_SINGLE), 1) \
+   : TREE_OPERAND (TREE_RANGE_CHECK (NODE, OACC_CACHE, OMP_TARGET_UPDATE), 0))
 
 #define OACC_PARALLEL_BODY(NODE) \
   TREE_OPERAND (OACC_PARALLEL_CHECK (NODE), 0)