[PATCH] Fix ubsan i?86 {add,sub,mul}v<mode>4 patterns

[Uros Bizjak]

On Tue, Mar 25, 2014 at 11:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 25, 2014 at 09:12:14AM +0100, Uros Bizjak wrote:
>> > The bootstrap-ubsan bootstrap revealed a problem with the
>> > {add,sub,mul}v<mode>4 patterns.  The problem is that they
>> > accept CONST_INT operands (because the instructions do accept immediates),
>> > but to model what the instruction actually does, we need to have both
>> > the value of the operand itself and it's sign extended value to
>> > 2x wider mode, but say (sign_extend:DI (const_int 5)) in the pattern is
>> > invalid RTL (found about this because e.g. num_sign_bit_copies returns
>> > bogus return values on (sign_extend:DI (const_int 0)) ).
>> > The following patch attempts to fix this by using two different define_insns
>> > for each, one that accepts everything but VOIDmode operands (i.e. usually
>> > register, memory, some SYMBOL_REFs/LABEL_REFs/CONSTs where we do have mode),
>> > one that accepts only CONST_INTs.  Hopefully at least the combiner will
>> > canonicalize the potential (sign_extend:DI (const_int N)) into
>> > just (const_int N) and thus the *v<mode>4_1 insns would match (plus the
>> > expander expands it that way too).
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, tested with
>> > i686-linux --with-build-config=bootstrap-ubsan bootstrap.  Ok for trunk?
>>
>> It looks to me that you will also need a corresponding predicate,
>> similar to x86_64_zext_operand that rejects sign-extended VOIDmode
>> operands where We constraint is used. That will also solve the problem
>> with combiner, which will be prohibited from combining VOIDmode
>> operands.
>>
>> Also, please use curly braces in multi-line preparation statements.
>
> Like this?  Or do you prefer a different name for the predicate?
> And, is it ok to use just one predicate for both {QI,HI}mode and
> {SI,DI}mode, or should I add separate predicates for "general_operand
> where GET_MODE (op) != VOIDmode" and "x86_64_general_operand where
> GET_MODE (op) != VOIDmode" and add a mode attribute for that?
> If so, what would be your preferred names for the 2 predicates and for the
> mode attribute?

The patch is OK in principle, but we could follow established practice
and use separate predicates - please see general_szext_operand mode
attribute definition.

I propose to use new "general_sext_operand" mode attribute that would
expand to "sext_operand" in QI/HImode case and "x86_64_sext_operand"
in SI/DImode case.

"sext_operand" should be defined similar to IOR part of
x86_64_zext_operand (using "immediate_operand" instead of
x86_64_zext_immediate_operand").

"x86_64_sext_operand" should be defined as "sext_operand", but should
use "x86_64_immediate_operand" in place of "immediate_operand".

(Looking at these definitions, it looks to me that other sign/zero
extend predicates also need to reject VOIDmode immediates, but let's
leave this to 4.9+).

Uros.