diff mbox

[1/2] PR77822

Message ID 20161117155347.GB21578@linux.vnet.ibm.com
State New
Headers show

Commit Message

Dominik Vogt Nov. 17, 2016, 3:53 p.m. UTC 
On Thu, Nov 17, 2016 at 04:53:03PM +0100, Dominik Vogt wrote:
> The following two patches fix PR 77822 on s390x for gcc-7.  As the
> macro doing the argument range checks can be used on other targets
> as well, I've put it in system.h (couldn't think of a better
> place; maybe rtl.h?).
> 
> Bootstrapped on s390x biarch, regression tested on s390x biarch
> and s390, all on a zEC12 with -march=zEC12.
> 
> Please check the commit messages for details.

Common code patch.

Ciao

Dominik ^_^  ^_^

Comments

Segher Boessenkool Nov. 18, 2016, 9:31 a.m. UTC | #1 
Hi Dominik,

On Thu, Nov 17, 2016 at 04:53:47PM +0100, Dominik Vogt wrote:
> +/* A convenience macro to determine whether a SIZE lies inclusively
> +   within [1, RANGE], POS lies inclusively within between
> +   [0, RANGE - 1] and the sum lies inclusively within [1, RANGE].  */
> +#define SIZE_POS_IN_RANGE(SIZE, POS, RANGE) \
> +  (IN_RANGE (POS, 0, (RANGE) - 1) \
> +   && IN_RANGE (SIZE, 1, RANGE) \
> +   && IN_RANGE ((SIZE) + (POS), 1, RANGE))

You should put parens around every use of SIZE, POS, RANGE -- there could
be a comma operator in the macro argument.

You don't check if SIZE+POS overflows / wraps around.  If you really don't
care about that, you can lose the

> +   && IN_RANGE (SIZE, 1, RANGE) \

part as well?


Segher
Dominik Vogt Nov. 18, 2016, 12:09 p.m. UTC | #2 
On Fri, Nov 18, 2016 at 03:31:40AM -0600, Segher Boessenkool wrote:
> Hi Dominik,
> 
> On Thu, Nov 17, 2016 at 04:53:47PM +0100, Dominik Vogt wrote:
> > +/* A convenience macro to determine whether a SIZE lies inclusively
> > +   within [1, RANGE], POS lies inclusively within between
> > +   [0, RANGE - 1] and the sum lies inclusively within [1, RANGE].  */
> > +#define SIZE_POS_IN_RANGE(SIZE, POS, RANGE) \
> > +  (IN_RANGE (POS, 0, (RANGE) - 1) \
> > +   && IN_RANGE (SIZE, 1, RANGE) \
> > +   && IN_RANGE ((SIZE) + (POS), 1, RANGE))
> 
> You should put parens around every use of SIZE, POS, RANGE -- there could
> be a comma operator in the macro argument.

Good point.  That wouldn't matter for a backend macro, but in
system.h it does.

> You don't check if SIZE+POS overflows / wraps around.  If you really don't
> care about that, you can lose the
> 
> > +   && IN_RANGE (SIZE, 1, RANGE) \
> 
> part as well?

The macro is _intended_ for checking the (possibly bogus)
arguments of zero_extract and sing_extract that combine may
generate.  SIZE is some unsigned value, but POS might be unsigned
or signed, and actually have a negative value (INTVAL
(operands[x]) or UINTVAL (operands[x]))), and RANGE is typically
64 or another small value.

Anyway, the macro should work for any inputs (except RANGE <= 0).

How about this:

--
/* A convenience macro to determine whether a SIZE lies inclusively
   within [1, UPPER], POS lies inclusively within between
   [0, UPPER - 1] and the sum lies inclusively within [1, UPPER].
   UPPER must be >= 1.  */
#define SIZE_POS_IN_RANGE(SIZE, POS, UPPER) \
  (IN_RANGE ((POS), 0, (unsigned HOST_WIDE_INT) (UPPER) - 1) \
   && IN_RANGE ((SIZE), 1, (unsigned HOST_WIDE_INT) (UPPER) \
                           - (unsigned HOST_WIDE_INT)(POS)))
--

IN_RANGE(POS...) makes sure that POS is a non-negative number
smaller than UPPER, so (UPPER) - (POS) does not wrap.  Or is there
some case that the new macro does not handle?

Ciao

Dominik ^_^  ^_^
Segher Boessenkool Nov. 18, 2016, 2:02 p.m. UTC | #3 
On Fri, Nov 18, 2016 at 01:09:24PM +0100, Dominik Vogt wrote:
> How about this:
> 
> --
> /* A convenience macro to determine whether a SIZE lies inclusively
>    within [1, UPPER], POS lies inclusively within between
>    [0, UPPER - 1] and the sum lies inclusively within [1, UPPER].
>    UPPER must be >= 1.  */
> #define SIZE_POS_IN_RANGE(SIZE, POS, UPPER) \
>   (IN_RANGE ((POS), 0, (unsigned HOST_WIDE_INT) (UPPER) - 1) \
>    && IN_RANGE ((SIZE), 1, (unsigned HOST_WIDE_INT) (UPPER) \
>                            - (unsigned HOST_WIDE_INT)(POS)))
                                                       ^ missing space here

> IN_RANGE(POS...) makes sure that POS is a non-negative number
> smaller than UPPER, so (UPPER) - (POS) does not wrap.  Or is there
> some case that the new macro does not handle?

I think it works fine.

With a name like UPPER, you may want to have SIZE_POS_IN_RANGE work like
IN_RANGE, i.e. UPPER is inclusive then.  Dunno.


Segher
Dominik Vogt Nov. 18, 2016, 3:29 p.m. UTC | #4 
On Fri, Nov 18, 2016 at 08:02:08AM -0600, Segher Boessenkool wrote:
> On Fri, Nov 18, 2016 at 01:09:24PM +0100, Dominik Vogt wrote:
> > IN_RANGE(POS...) makes sure that POS is a non-negative number
> > smaller than UPPER, so (UPPER) - (POS) does not wrap.  Or is there
> > some case that the new macro does not handle?
> 
> I think it works fine.
> 
> With a name like UPPER, you may want to have SIZE_POS_IN_RANGE work like
> IN_RANGE, i.e. UPPER is inclusive then.  Dunno.

Yeah, maybe rather call it RANGE to avoid too much similarity.
Some name that is so vague that one has to read the documentation.
I'll post an updated patch later.

Ciao

Dominik ^_^  ^_^
diff mbox

Patch

diff --git a/gcc/system.h b/gcc/system.h
index 8c6127c..cc68f07 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -316,6 +316,14 @@  extern int errno;
   ((unsigned HOST_WIDE_INT) (VALUE) - (unsigned HOST_WIDE_INT) (LOWER) \
    <= (unsigned HOST_WIDE_INT) (UPPER) - (unsigned HOST_WIDE_INT) (LOWER))
 
+/* A convenience macro to determine whether a SIZE lies inclusively
+   within [1, RANGE], POS lies inclusively within between
+   [0, RANGE - 1] and the sum lies inclusively within [1, RANGE].  */
+#define SIZE_POS_IN_RANGE(SIZE, POS, RANGE) \
+  (IN_RANGE (POS, 0, (RANGE) - 1) \
+   && IN_RANGE (SIZE, 1, RANGE) \
+   && IN_RANGE ((SIZE) + (POS), 1, RANGE))
+
 /* Infrastructure for defining missing _MAX and _MIN macros.  Note that
    macros defined with these cannot be used in #if.  */