On 12/19/2017 12:13 PM, Richard Sandiford wrote:
> Richard Sandiford <richard.sandif...@linaro.org> writes:
>> Richard Biener <richard.guent...@gmail.com> writes:
>>> On Fri, Dec 15, 2017 at 1:48 AM, Richard Sandiford
>>> <richard.sandif...@linaro.org> wrote:
>>>> Richard Biener <richard.guent...@gmail.com> writes:
>>>>> On Mon, Nov 20, 2017 at 10:02 PM, Richard Sandiford
>>>>> <richard.sandif...@linaro.org> wrote:
>>>>>> Richard Biener <richard.guent...@gmail.com> writes:
>>>>>>> On Thu, Oct 26, 2017 at 2:06 PM, Richard Biener
>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>> On Mon, Oct 23, 2017 at 1:25 PM, Richard Sandiford
>>>>>>>> <richard.sandif...@linaro.org> wrote:
>>>>>>>>> This patch adds a stub helper routine to provide the mode
>>>>>>>>> of a scalar shift amount, given the mode of the values
>>>>>>>>> being shifted.
>>>>>>>>>
>>>>>>>>> One long-standing problem has been to decide what this mode
>>>>>>>>> should be for arbitrary rtxes (as opposed to those directly
>>>>>>>>> tied to a target pattern). Is it the mode of the shifted
>>>>>>>>> elements? Is it word_mode? Or maybe QImode? Is it whatever
>>>>>>>>> the corresponding target pattern says? (In which case what
>>>>>>>>> should the mode be when the target doesn't have a pattern?)
>>>>>>>>>
>>>>>>>>> For now the patch picks word_mode, which should be safe on
>>>>>>>>> all targets but could perhaps become suboptimal if the helper
>>>>>>>>> routine is used more often than it is in this patch. As it
>>>>>>>>> stands the patch does not change the generated code.
>>>>>>>>>
>>>>>>>>> The patch also adds a helper function that constructs rtxes
>>>>>>>>> for constant shift amounts, again given the mode of the value
>>>>>>>>> being shifted. As well as helping with the SVE patches, this
>>>>>>>>> is one step towards allowing CONST_INTs to have a real mode.
>>>>>>>>
>>>>>>>> I think gen_shift_amount_mode is flawed and while encapsulating
>>>>>>>> constant shift amount RTX generation into a gen_int_shift_amount
>>>>>>>> looks good to me I'd rather have that ??? in this function (and
>>>>>>>> I'd use the mode of the RTX shifted, not word_mode...).
>>>>>>
>>>>>> OK. I'd gone for word_mode because that's what expand_binop uses
>>>>>> for CONST_INTs:
>>>>>>
>>>>>> op1_mode = (GET_MODE (op1) != VOIDmode
>>>>>> ? as_a <scalar_int_mode> (GET_MODE (op1))
>>>>>> : word_mode);
>>>>>>
>>>>>> But using the inner mode should be fine too. The patch below does that.
>>>>>>
>>>>>>>> In the end it's up to insn recognizing to convert the op to the
>>>>>>>> expected mode and for generic RTL it's us that should decide
>>>>>>>> on the mode -- on GENERIC the shift amount has to be an
>>>>>>>> integer so why not simply use a mode that is large enough to
>>>>>>>> make the constant fit?
>>>>>>
>>>>>> ...but I can do that instead if you think it's better.
>>>>>>
>>>>>>>> Just throwing in some comments here, RTL isn't my primary
>>>>>>>> expertise.
>>>>>>>
>>>>>>> To add a little bit - shift amounts is maybe the only(?) place
>>>>>>> where a modeless CONST_INT makes sense! So "fixing"
>>>>>>> that first sounds backwards.
>>>>>>
>>>>>> But even here they have a mode conceptually, since out-of-range shift
>>>>>> amounts are target-defined rather than undefined. E.g. if the target
>>>>>> interprets the shift amount as unsigned, then for a shift amount
>>>>>> (const_int -1) it matters whether the mode is QImode (and so we're
>>>>>> shifting by 255) or HImode (and so we're shifting by 65535.
>>>>>
>>>>> I think RTL is well-defined (at least I hope so ...) and machine
>>>>> constraints
>>>>> need to be modeled explicitely (like embedding an implicit bit_and in
>>>>> shift patterns).
>>>>
>>>> Well, RTL is well-defined in the sense that if you have
>>>>
>>>> (ashift X (foo:HI ...))
>>>>
>>>> then the shift amount must be interpreted as HImode rather than some
>>>> other mode. The problem here is to define a default choice of mode for
>>>> const_ints, in cases where the shift is being created out of the blue.
>>>>
>>>> Whether the shift amount is effectively signed or unsigned isn't defined
>>>> by RTL without SHIFT_COUNT_TRUNCATED, since the choice only matters for
>>>> out-of-range values, and the behaviour for out-of-range RTL shifts is
>>>> specifically treated as target-defined without SHIFT_COUNT_TRUNCATED.
>>>>
>>>> I think the revised patch does implement your suggestion of using the
>>>> integer equivalent of the inner mode as the default, but we need to
>>>> decide whether to go with it, go with the original word_mode approach
>>>> (taken from existing expand_binop code) or something else. Something
>>>> else could include the widest supported integer mode, so that we never
>>>> change the value.
>>>
>>> I guess it's pretty arbitrary what we choose (but we might need to adjust
>>> targets?). For something like this an appealing choice would be sth
>>> that is host and target idependent, like [u]int32_t or given CONST_INT
>>> is always 64bits now and signed int64_t aka HOST_WIDE_INT (bad
>>> name now). That means it's the "infinite precision" thing that fits
>>> into CONST_INT ;)
>>
>> Sounds OK to me. How about the attached?
>
> Taking MAX_FIXED_MODE_SIZE into account was bogus, since we'd then just
> fail to find a mode. This version has survived the full cross-target
> testing. Also bootstrapped & regression-tested on aarch64-linux-gnu,
> x86_64-linux-gnu and powerpc64-linux-gnu. OK to install?
>
> At this stage this is the patch that is holding up the rest of the
> approved ones.
>
> Thanks,
> Richard
>
>
> 2017-12-19 Richard Sandiford <richard.sandif...@linaro.org>
> Alan Hayward <alan.hayw...@arm.com>
> David Sherwood <david.sherw...@arm.com>
>
> gcc/
> * emit-rtl.h (gen_int_shift_amount): Declare.
> * emit-rtl.c (gen_int_shift_amount): New function.
> * asan.c (asan_emit_stack_protection): Use gen_int_shift_amount
> instead of GEN_INT.
> * calls.c (shift_return_value): Likewise.
> * cse.c (fold_rtx): Likewise.
> * dse.c (find_shift_sequence): Likewise.
> * expmed.c (init_expmed_one_mode, store_bit_field_1, expand_shift_1)
> (expand_shift, expand_smod_pow2): Likewise.
> * lower-subreg.c (shift_cost): Likewise.
> * optabs.c (expand_superword_shift, expand_doubleword_mult)
> (expand_unop, expand_binop, shift_amt_for_vec_perm_mask)
> (expand_vec_perm_var): Likewise.
> * simplify-rtx.c (simplify_unary_operation_1): Likewise.
> (simplify_binary_operation_1): Likewise.
> * combine.c (try_combine, find_split_point, force_int_to_mode)
> (simplify_shift_const_1, simplify_shift_const): Likewise.
> (change_zero_ext): Likewise. Use simplify_gen_binary.
>
OK.
jeff
