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.
> > + if (val_so_far > (unsigned HOST_WIDE_INT) HOST_WIDE_INT_MAX)
> > + {
> > + wide_int wval_so_far
> > + = wi::uhwi (val_so_far,
> > + GET_MODE_PRECISION (as_a <scalar_mode> (nmode)));
> > + c = immed_wide_int_const (wval_so_far, nmode);
> > + }
> > + else
> > + c = gen_int_mode (val_so_far, nmode);
Jakub
