Re: [PATCH v3 ] i386: Add ix86_expand_integer_cst_argument

2024-11-13 Thread Jakub Jelinek 
On Wed, Nov 13, 2024 at 09:22:45AM +0100, Richard Biener wrote:
> While I'm far from an expert here this doesn't look right and instead the
> const_0_to_255_operand looks bogus to me in not properly taking into
> account 'mode'.

I think the bug is in use of const_0_to_255* predicates with QImode or
mode-less operands, i.e.
grep const_0_to_255 *.md | grep -v ':SI\|define_predicate'
i386.md:(match_operand:QI 3 "const_0_to_255_operand")) 0)))]
i386.md:(match_operand:QI 4 "const_0_to_255_operand")) 0)))]
i386.md:(match_operand:QI 3 "const_0_to_255_operand")) 0)))]
i386.md:(match_operand:QI 4 "const_0_to_255_operand")) 0)))]
i386.md:  (match_operand:QI 2 "const_0_to_255_operand")
i386.md:  (match_operand:QI 3 "const_0_to_255_operand")))
sse.md:   (match_operand 2 "const_0_to_255_operand")))
sse.md:   (match_operand 2 "const_0_to_255_operand")
sse.md:   (match_operand 3 "const_0_to_255_operand")]
sse.md:   (match_operand 3 "const_0_to_255_operand")
sse.md:   (match_operand 4 "const_0_to_255_operand")]
sse.md: (match_operand 2 "const_0_to_255_operand")]
sse.md: (match_operand 2 "const_0_to_255_operand")]
sse.md:(match_operand 3 "const_0_to_255_operand")]
sse.md:(match_operand 3 "const_0_to_255_operand")]
sse.md: (match_operand 2 "const_0_to_255_operand")]
With QImode, const_0_to_255_operand makes no sense, valid values are
-128..127.  So, if QImode in that case is right, just use const_int_operand.
And obviously the INTVAL uses on that operand need to be prepared for the
value being -128..127.  I believe during expansion if the match_operand is
QImode that it will be turned into -128..127.
The VOIDmode ones even worse because I think nothing will truncate the
constants in there.  So, either turn those into QImode with
const_int_operand or SImode with const_0_to_255_operand, but in the latter
case change also i386-builtin.def to pass in SImode (UINT) operand rather than
QImode.

Basically, with QImode there can't be any in-range checking for error
diagnostics, all values are valid.  If we want checking, the builtin
should have SImode or similar argument (or whatever the intrinsic argument
type is) and so should be the match_operand.

E.g.
#include 

__mmask64 foo (__mmask64 x) { return _kshiftli_mask64 (x, 257); }
errors with clang/icx, warns with icc but is silently accepted by gcc.
That looks wrong to me.
extern __inline __mmask64
__attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
_kshiftli_mask64 (__mmask64 __A, unsigned int __B)
{
  return (__mmask64) __builtin_ia32_kshiftlidi ((__mmask64) __A,
(__mmask8) __B);
}
The second argument is unsigned int shift count, so shouldn't be cast
to __mmask8 which is for masks, you don't shift a mask by mask but mask by
unsigned integer.

Jakub

Re: [PATCH v3 ] i386: Add ix86_expand_integer_cst_argument

2024-11-13 Thread Richard Biener 
On Wed, Nov 13, 2024 at 6:22 AM H.J. Lu  wrote:
>
> On Wed, Nov 13, 2024 at 11:25 AM H.J. Lu  wrote:
> >
> > On Wed, Nov 13, 2024 at 10:23 AM Hongtao Liu  wrote:
> > >
> > > On Wed, Nov 13, 2024 at 8:29 AM H.J. Lu  wrote:
> > > >
> > > > On Wed, Nov 13, 2024 at 5:57 AM H.J. Lu  wrote:
> > > > >
> > > > > On Tue, Nov 12, 2024 at 9:30 PM Richard Biener
> > > > >  wrote:
> > > > > >
> > > > > > On Tue, Nov 12, 2024 at 1:49 PM H.J. Lu  wrote:
> > > > > > >
> > > > > > > When passing 0xff as an unsigned char function argument, the C 
> > > > > > > frontend
> > > > > > > promotion will promote it to int:
> > > > > > >
> > > > > > >  > > > > > > int> constant 255>
> > > > > > >
> > > > > > > and expand_normal always returns the rtx value using the 
> > > > > > > sign-extended
> > > > > > > representation,
> > > > > > >
> > > > > > > (const_int 255 [0xff])
> > > > > > >
> > > > > > > If the C frontend doesn't promote unsigned char to int, 
> > > > > > > expand_normal will
> > > > > > > get
> > > > > > >
> > > > > > >  > > > > > > unsigned char > co
> > > > > > > nstant 255>
> > > > > > >
> > > > > > > and return
> > > > > > >
> > > > > > > (const_int -1 [0x])
> > > > > >
> > > > > > that looks wrong to me, but in other places we ensure
> > > > > > to use trunc_int_for_mode (), not some odd function like
> > > > > > you introduce here?
> > > > >
> > > > > I opened:
> > > > >
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117547
> > > > >
> > > >
> > > > Here is the v2 patch with reference to PR target/117547.
> > > >
> > > > When passing 0xff as an unsigned char function argument with the C 
> > > > frontend
> > > > promotion, expand_normal gets
> > > >
> > > >  
> > > > constant 255>
> > > >
> > > > and returns the rtx value using the sign-extended representation:
> > > >
> > > > (const_int 255 [0xff])
> > > >
> > > > Without the C frontend promotion, expand_normal gets
> > > >
> > > >  > > > char > co
> > > > nstant 255>
> > > >
> > > > and returns
> > > >
> > > >  (const_int -1 [0x])
> > > >
> > > > which doesn't work with the IN_RANGE predicates.  Extract the 
> > > > 8-bit/16-bit
> > > > integer constants to always return
> > > >
> > > > (const_int 255 [0xff])
> > > >
> > > > so that the return value can be used with the IN_RANGE predicates, like
> > > > const_0_to_255_operand, without the C frontend promotion.
> > > +  if (TREE_CODE (arg) == INTEGER_CST)
> > > +{
> > > +  tree type = TREE_TYPE (arg);
> > > +  if (INTEGRAL_TYPE_P (type)
> > > + && TYPE_PRECISION (type) < TYPE_PRECISION (integer_type_node))
> > > +   {
> > > + HOST_WIDE_INT cst = TREE_INT_CST_LOW (arg);
> > > + return GEN_INT (cst);
> > > +   }
> > > +}
> > > +
> > >
> > > Shouldn't we guard this with TYPE_UNSIGNED (type)?
> >
> > Good idea.  I will add it.
> >
> > > I'm also worried that GEN_INT may trigger ICE like PR96262.
> > > Can we use gen_int_mode instead, or we must use GEN_INT here?
> >
> > gen_int_mode (255, QImode) returns
> >
> > (const_int -1 [0x])
> >
> > unless we use gen_int_mode (255, SImode)
> >
>
> Here is the v3 patch which adds the check with TYPE_UNSIGNED (type).
>
> OK for master?

While I'm far from an expert here this doesn't look right and instead the
const_0_to_255_operand looks bogus to me in not properly taking into
account 'mode'.

Richard.

> H.J.
> ---
> When passing 0xff as an unsigned char function argument with the C frontend
> promotion, expand_normal gets
>
>  constant 
> 255>
>
> and returns the rtx value using the sign-extended representation:
>
> (const_int 255 [0xff])
>
> Without the C frontend promotion, expand_normal gets
>
>  
> co
> nstant 255>
>
> and returns
>
>  (const_int -1 [0x])
>
> which doesn't work with the IN_RANGE predicates.  Extract the unsigned
> char/short integer constants to always return
>
> (const_int 255 [0xff])
>
> so that the return value can be used with the IN_RANGE predicates, like
> const_0_to_255_operand, without the C frontend promotion.
>
> PR target/117547
> * config/i386/i386-expand.cc (ix86_expand_integer_cst_argument):
> New function.
> (ix86_expand_args_builtin): Call ix86_expand_integer_cst_argument
> to expand the argument before calling fixup_modeless_constant.
> (ix86_expand_round_builtin): Likewise.
> (ix86_expand_special_args_builtin): Likewise.
> (ix86_expand_builtin): Likewise.
>
> --
> H.J.