Re: [PATCH] Fix expand_mult_const (PR middle-end/87138)

[Richard Biener <rguenther at suse dot de>]

On Thu, 30 Aug 2018, Jakub Jelinek wrote:

> On Thu, Aug 30, 2018 at 09:31:01AM +0200, Richard Biener wrote:
> > > The following testcase is miscompiled, because expand_mult_const adds an
> > > incorrect REG_EQUAL note to a temporary pseudo.  In the testcase, we
> > > do an unsigned __int128 multiplication of a variable by
> > > 0x7fffffffffffffff, which is determined to be best performed as
> > > shift left by 63 (multiplication by 0x8000000000000000U) followed by
> > > subtraction, i.e. (x << 63) - x.  The val_so_far is tracked in an UHWI and
> > > is necessarily always non-negative even because the caller ensures that,
> > > if (is_neg && mode_bitsize > HOST_BITS_PER_WIDE_INT) then it performs
> > > expand_mult_const on the negation and negates afterwards the result.
> > > The problem is that it calls gen_int_mode, where the argument is signed hwi
> > > (well, these days poly_int64).  That is fine for DImode multiplication, we
> > > don't really care about bits above the mode, but for TImode multiplication
> > > it is significant.  Without this patch we emit in that case:
> > >      (expr_list:REG_EQUAL (mult:TI (reg/v:TI 85 [ x ])
> > >             (const_int -9223372036854775808 [0x8000000000000000]))
> > > but that is for TImode actually multiplication by
> > > 0xffffffffffffffff8000000000000000
> > > rather than
> > > 0x00000000000000008000000000000000, so we need to emit
> > > 	    (const_wide_int 0x08000000000000000)
> > > instead.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > > trunk and after a while for 8.3?
> > 
> > Ugh.  I wonder if we should add a
> > 
> > rtx gen_int_mode (poly_uint64 c, machine_mode mode)
> > 
> > and assert that the topmost bit is not set if GET_MODE_PRECISION (mode) > 64?
> > 
> > But I guess passing unsigned HOST_WIDE_INT will make this ambiguous.
> > So maybe a unsigned HOST_WIDE_INT overload instead.
> 
> I can try that, but I think there might be false positives too,
> I think we use uhwi in many spots for whatever reason (e.g. defined overflow
> behavior) and still want to treat it as shwi in the end.  Plus testsuite
> coverage for TImode arithmetics is limited.
> 
> > Just to catch the possibly very many places where things can go wrong...
> > 
> > I also wonder why callers have to decide whether to use a CONST_INT,
> > CONST_WIDE_INT or CONST_DOUBLE and why we cannot have a single
> > interface here, eventually taking a sign in addition to the mode.
> 
> We do have it, it is the immed_wide_int_const.  The then block can be used
> unconditionally, the reason I've made that conditional was to optimize for
> the 99.9% common case, where we'd construct a wide_int, call
> immed_wide_int_const which would do some check and call gen_int_mode again.
> 
> If you think for maintainance it is better to just do that unconditionally,
> I can certainly retest.

Yes, I think so.  It also makes it explicit wheter we want an unsigned
or signed value.

Richard.