[rs6000] PR59708, Fix operand ordering for 128-bit andc/orc

The following patch fixes testsuite failures 
builtin-arith-overflow-[10,11].c. In the case where 
rs6000_split_logical() was being called the operands were swapped such 
that the wrong operand was being complemented.

Bootstrap/tested on powerpc64-linux with no new regressions. Ok for 
trunk (and 4.9/4.8 after bootstrap/regtest)?

2014-12-18  Pat Haugen  <pthaugen@us.ibm.com>

         * config/rs6000/rs6000.md (boolc<mode>3_internal1): Fix operand
         ordering.

Hi,

On Thu, Dec 18, 2014 at 02:48:46PM -0600, Pat Haugen wrote:
>  ;; 128-bit ANDC/ORC
> +;;   In the case where rs6000_split_logical is called, the NOT'd operand
> +;;   must be opnd1 in order for the split insns to be recognized.

So fix rs6000_split_logical?

>  (define_insn_and_split "*boolc<mode>3_internal1"
>    [(set (match_operand:BOOL_128 0 "vlogical_operand" "=<BOOL_REGS_OUTPUT>")
>  	(match_operator:BOOL_128 3 "boolean_operator"
>  	 [(not:BOOL_128
> -	   (match_operand:BOOL_128 2 "vlogical_operand" "<BOOL_REGS_OP1>"))
> -	  (match_operand:BOOL_128 1 "vlogical_operand" "<BOOL_REGS_OP2>")]))]
> +	   (match_operand:BOOL_128 1 "vlogical_operand" "<BOOL_REGS_OP1>"))
> +	  (match_operand:BOOL_128 2 "vlogical_operand" "<BOOL_REGS_OP2>")]))]

This fixes the previously wrong BOOL_REGS_OP[12], you should mention
that somewhere :-)

>    "TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND)"
>  {
>    if (TARGET_VSX && vsx_register_operand (operands[0], <MODE>mode))
> -    return "xxl%q3 %x0,%x1,%x2";
> +    return "xxl%q3 %x0,%x2,%x1";
>  
>    if (TARGET_ALTIVEC && altivec_register_operand (operands[0], <MODE>mode))
> -    return "v%q3 %0,%1,%2";
> +    return "v%q3 %0,%2,%1";

Please let's keep operands in assembler order if at all possible.


Segher

On Fri, Dec 19, 2014 at 10:54:22AM -0600, Segher Boessenkool wrote:
> On Thu, Dec 18, 2014 at 02:48:46PM -0600, Pat Haugen wrote:
> >  ;; 128-bit ANDC/ORC
> > +;;   In the case where rs6000_split_logical is called, the NOT'd operand
> > +;;   must be opnd1 in order for the split insns to be recognized.
> 
> So fix rs6000_split_logical?

That is, the call to it -- if operand[2] has the NOT, it should be
false, false, true instead of false, true, false.


Segher

On 12/19/2014 01:50 PM, Segher Boessenkool wrote:
> On Fri, Dec 19, 2014 at 10:54:22AM -0600, Segher Boessenkool wrote:
>> >On Thu, Dec 18, 2014 at 02:48:46PM -0600, Pat Haugen wrote:
>>> > >  ;; 128-bit ANDC/ORC
>>> > >+;;   In the case where rs6000_split_logical is called, the NOT'd operand
>>> > >+;;   must be opnd1 in order for the split insns to be recognized.
>> >
>> >So fix rs6000_split_logical?
> That is, the call to it -- if operand[2] has the NOT, it should be
> false, false, true instead of false, true, false.
>
Yes, that was my first attempt at fixing this problem, but ended up with 
an unrecognized insn for the following. Which is why I switched my 
approach to swapping operands in the patch.


builtin-arith-overflow-7.c:80:1: error: unrecognizable insn:
  }
  ^
(insn 537 536 538 35 (set (reg:DI 10 10)
         (and:DI (reg:DI 8 8)
             (not:DI (reg:DI 30 30)))) builtin-arith-overflow-7.c:76 -1
      (nil))

The canonical form has the NOT first.  If the second operand needs the
NOT, the operands need to be reversed when the new insn is created, in
rs6000_split_logical, not in the pattern.

Thanks, David


On Fri, Dec 19, 2014 at 4:27 PM, Pat Haugen <pthaugen@linux.vnet.ibm.com> wrote:
> On 12/19/2014 01:50 PM, Segher Boessenkool wrote:
>>
>> On Fri, Dec 19, 2014 at 10:54:22AM -0600, Segher Boessenkool wrote:
>>>
>>> >On Thu, Dec 18, 2014 at 02:48:46PM -0600, Pat Haugen wrote:
>>>>
>>>> > >  ;; 128-bit ANDC/ORC
>>>> > >+;;   In the case where rs6000_split_logical is called, the NOT'd
>>>> > > operand
>>>> > >+;;   must be opnd1 in order for the split insns to be recognized.
>>>
>>> >
>>> >So fix rs6000_split_logical?
>>
>> That is, the call to it -- if operand[2] has the NOT, it should be
>> false, false, true instead of false, true, false.
>>
> Yes, that was my first attempt at fixing this problem, but ended up with an
> unrecognized insn for the following. Which is why I switched my approach to
> swapping operands in the patch.
>
>
> builtin-arith-overflow-7.c:80:1: error: unrecognizable insn:
>  }
>  ^
> (insn 537 536 538 35 (set (reg:DI 10 10)
>         (and:DI (reg:DI 8 8)
>             (not:DI (reg:DI 30 30)))) builtin-arith-overflow-7.c:76 -1
>      (nil))
>