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.
