On Thu, Jun 26, 2014 at 11:06:26AM +1000, Kugan wrote:
> >> Since our aim is to perform single bit checks, why don’t we just use
> >> this representation internally (i.e. _rtx->unchanging = 1 if SRP_SIGNED
> >> and _rtx->volatil = 1 if SRP_UNSIGNED). As for SUBREG_PROMOTED_SIGNED_P,
> >> we still have to return -1 or 1 depending on SRP_POINTER or SRP_UNSIGNED.
> >
> > Why don't you make SUBREG_PROMOTED_UNSIGNED_P just return 0/1 (i.e. the
> > single bit), and for places where it would like to match both
> > SRP_UNSIGNED and SRP_POINTER use SUBREG_PROMOTED_GET () & SRP_UNSIGNED
> > or so?
>
> If we use SUBREG_PROMOTED_GET () & SRP_UNSIGNED, we will miss
> the case SRP_SIGNED_AND_UNSIGNED. Though this is not wrong, we might
> miss some optimization opportunities here. We can however use
> (SUBREG_PROMOTED_GET () != SRP_SIGNED) if you like this. Other option is
> to define another macro that explicilty says some think like
> SUBREG_PROMOTED_POINTER_OR_UNSIGNED_P.
Ok, sure, if you want to make the test pass for SRP_UNSIGNED, SRP_POINTER
and SRP_UNSIGNED_AND_SIGNED, then != SRP_SIGNED is the right thing.
What I wanted is make SUBREG_PROMOTED_UNSIGNED_P be a 0/1 again.
> >> --- a/gcc/ifcvt.c
> >> +++ b/gcc/ifcvt.c
> >> @@ -1448,8 +1448,11 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx
> >> x, enum rtx_code code,
> >> || byte_vtrue != byte_vfalse
> >> || (SUBREG_PROMOTED_VAR_P (vtrue)
> >> != SUBREG_PROMOTED_VAR_P (vfalse))
> >> - || (SUBREG_PROMOTED_UNSIGNED_P (vtrue)
> >> - != SUBREG_PROMOTED_UNSIGNED_P (vfalse)))
> >> + || ((SUBREG_PROMOTED_UNSIGNED_P (vtrue)
> >> + != SUBREG_PROMOTED_UNSIGNED_P (vfalse))
> >> + && (SUBREG_PROMOTED_SIGNED_P (vtrue)
> >> + != SUBREG_PROMOTED_SIGNED_P (vfalse))))
> >
> > Shouldn't this be SUBREG_PROMOTED_GET (vtrue) != SUBREG_PROMOTED_GET
> > (vfalse) ?
>
> The reason why I checked like this to cover one side with
> SRP_SIGNED_AND_UNSIGNED and other with SRP_SIGNED or SRP_UNSIGNED. If
> we check SUBREG_PROMOTED_GET (vtrue) != SUBREG_PROMOTED_GET (vfalse) we
> will miss that.
What you have above is just wrong though. Either you need to make sure the
flags are the same (i.e. GET != GET), and keep the SET a few lines below as
is, or you would allow (some?) mismatches of the promotion flags,
but in that case you'd need to deal with it in the SET conservatively.
Like, if one is SRP_SIGNED_AND_UNSIGNED and another one is just
SRP_SIGNED or just SRP_UNSIGNED, you'd use the simpler one, if one
is promoted and another one is not, you'd not make the SUBREG promoted at
all, etc. Not worth it IMHO, at least not for now.
>
> >> +
> >> +/* Predicate to check if RTX of SUBREG_PROMOTED_VAR_P() is promoted
> >> + for UNSIGNED type. In case of SRP_POINTER, SUBREG_PROMOTED_UNSIGNED_P
> >> + returns -1 as this is in most cases handled like unsigned extension,
> >> + except for generating instructions where special code is emitted for
> >> + (ptr_extend insns) on some architectures. */
> >> #define SUBREG_PROMOTED_UNSIGNED_P(RTX) \
> >> - ((RTL_FLAG_CHECK1 ("SUBREG_PROMOTED_UNSIGNED_P", (RTX),
> >> SUBREG)->volatil) \
> >> - ? -1 : (int) (RTX)->unchanging)
> >> + ((((RTL_FLAG_CHECK1 ("SUBREG_PROMOTED_UNSIGNED_P", (RTX),
> >> SUBREG)->volatil)\
> >> + + (RTX)->unchanging) == 0) ? -1 : ((RTX)->volatil == 1))
> >> +
> >> +/* Checks if RTX of SUBREG_PROMOTED_VAR_P() is promotd for given SIGN. */
> >> +#define SUBREG_CHECK_PROMOTED_SIGN(RTX, SIGN) \
> >
> > Use space rather than tab. Also, why do we need this macro?
> > Can't you just use SUBREG_PROMOTED_GET () == sign ? I mean, sign in that
> > case is typically just 0 or 1.
>
> Again I wanted to cover SRP_SIGNED_AND_UNSIGNED as well in this case.
Ah, ok. It is fine as is (with the whitespace change).
Jakub
