Remove ix86_legitimate_combined_insn

From INSTRUCTION EXCEPTION SPECIFICATION section in Intel software
developer manual volume 2, only legacy SSE instructions with memory
operand not 16-byte aligned get General Protection fault.  There is
no need to check 1, 2, 4, 8 byte alignments.  Since x86 backend has
accurate constraints and predicates for 16-byte alignment, we can
remove ix86_legitimate_combined_insn.

Tested on x86-64.  OK for trunk?

H.J.
---
	* config/i386/i386.c (ix86_legitimate_combined_insn): Removed.
	(TARGET_LEGITIMATE_COMBINED_INSN): Likewise.
	(ix86_expand_special_args_builtin): Replace
	ix86_legitimate_combined_insn with vector_memory_operand in
	comments.
---
 gcc/config/i386/i386.c | 96 ++------------------------------------------------
 1 file changed, 2 insertions(+), 94 deletions(-)

On Tue, Apr 19, 2016 at 4:49 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>
> From INSTRUCTION EXCEPTION SPECIFICATION section in Intel software
> developer manual volume 2, only legacy SSE instructions with memory
> operand not 16-byte aligned get General Protection fault.  There is
> no need to check 1, 2, 4, 8 byte alignments.  Since x86 backend has
> accurate constraints and predicates for 16-byte alignment, we can
> remove ix86_legitimate_combined_insn.
>
> Tested on x86-64.  OK for trunk?

No. This function also handles cases where invalid hard register gets
propagated into the insn during the combine pass, leading to spill
failure later.

Uros.

> H.J.
> ---
>         * config/i386/i386.c (ix86_legitimate_combined_insn): Removed.
>         (TARGET_LEGITIMATE_COMBINED_INSN): Likewise.
>         (ix86_expand_special_args_builtin): Replace
>         ix86_legitimate_combined_insn with vector_memory_operand in
>         comments.
> ---
>  gcc/config/i386/i386.c | 96 ++------------------------------------------------
>  1 file changed, 2 insertions(+), 94 deletions(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index e056f68..a66cfc4 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -7288,95 +7288,6 @@ ix86_return_pops_args (tree fundecl, tree funtype, int size)
>
>    return 0;
>  }
> -
> -/* Implement the TARGET_LEGITIMATE_COMBINED_INSN hook.  */
> -
> -static bool
> -ix86_legitimate_combined_insn (rtx_insn *insn)
> -{
> -  /* Check operand constraints in case hard registers were propagated
> -     into insn pattern.  This check prevents combine pass from
> -     generating insn patterns with invalid hard register operands.
> -     These invalid insns can eventually confuse reload to error out
> -     with a spill failure.  See also PRs 46829 and 46843.  */
> -  if ((INSN_CODE (insn) = recog (PATTERN (insn), insn, 0)) >= 0)
> -    {
> -      int i;
> -
> -      extract_insn (insn);
> -      preprocess_constraints (insn);
> -
> -      int n_operands = recog_data.n_operands;
> -      int n_alternatives = recog_data.n_alternatives;
> -      for (i = 0; i < n_operands; i++)
> -       {
> -         rtx op = recog_data.operand[i];
> -         machine_mode mode = GET_MODE (op);
> -         const operand_alternative *op_alt;
> -         int offset = 0;
> -         bool win;
> -         int j;
> -
> -         /* For pre-AVX disallow unaligned loads/stores where the
> -            instructions don't support it.  */
> -         if (!TARGET_AVX
> -             && VECTOR_MODE_P (mode)
> -             && misaligned_operand (op, mode))
> -           {
> -             unsigned int min_align = get_attr_ssememalign (insn);
> -             if (min_align == 0
> -                 || MEM_ALIGN (op) < min_align)
> -               return false;
> -           }
> -
> -         /* A unary operator may be accepted by the predicate, but it
> -            is irrelevant for matching constraints.  */
> -         if (UNARY_P (op))
> -           op = XEXP (op, 0);
> -
> -         if (SUBREG_P (op))
> -           {
> -             if (REG_P (SUBREG_REG (op))
> -                 && REGNO (SUBREG_REG (op)) < FIRST_PSEUDO_REGISTER)
> -               offset = subreg_regno_offset (REGNO (SUBREG_REG (op)),
> -                                             GET_MODE (SUBREG_REG (op)),
> -                                             SUBREG_BYTE (op),
> -                                             GET_MODE (op));
> -             op = SUBREG_REG (op);
> -           }
> -
> -         if (!(REG_P (op) && HARD_REGISTER_P (op)))
> -           continue;
> -
> -         op_alt = recog_op_alt;
> -
> -         /* Operand has no constraints, anything is OK.  */
> -         win = !n_alternatives;
> -
> -         alternative_mask preferred = get_preferred_alternatives (insn);
> -         for (j = 0; j < n_alternatives; j++, op_alt += n_operands)
> -           {
> -             if (!TEST_BIT (preferred, j))
> -               continue;
> -             if (op_alt[i].anything_ok
> -                 || (op_alt[i].matches != -1
> -                     && operands_match_p
> -                         (recog_data.operand[i],
> -                          recog_data.operand[op_alt[i].matches]))
> -                 || reg_fits_class_p (op, op_alt[i].cl, offset, mode))
> -               {
> -                 win = true;
> -                 break;
> -               }
> -           }
> -
> -         if (!win)
> -           return false;
> -       }
> -    }
> -
> -  return true;
> -}
>
>  /* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
>
> @@ -39859,7 +39770,7 @@ ix86_expand_special_args_builtin (const struct builtin_description *d,
>              on it.  Try to improve it using get_pointer_alignment,
>              and if the special builtin is one that requires strict
>              mode alignment, also from it's GET_MODE_ALIGNMENT.
> -            Failure to do so could lead to ix86_legitimate_combined_insn
> +            Failure to do so could lead to vector_memory_operand
>              rejecting all changes to such insns.  */
>           unsigned int align = get_pointer_alignment (arg);
>           if (aligned_mem && align < GET_MODE_ALIGNMENT (tmode))
> @@ -39915,7 +39826,7 @@ ix86_expand_special_args_builtin (const struct builtin_description *d,
>                  on it.  Try to improve it using get_pointer_alignment,
>                  and if the special builtin is one that requires strict
>                  mode alignment, also from it's GET_MODE_ALIGNMENT.
> -                Failure to do so could lead to ix86_legitimate_combined_insn
> +                Failure to do so could lead to vector_memory_operand
>                  rejecting all changes to such insns.  */
>               unsigned int align = get_pointer_alignment (arg);
>               if (aligned_mem && align < GET_MODE_ALIGNMENT (mode))
> @@ -54472,9 +54383,6 @@ ix86_addr_space_zero_address_valid (addr_space_t as)
>  #undef TARGET_RETURN_POPS_ARGS
>  #define TARGET_RETURN_POPS_ARGS ix86_return_pops_args
>
> -#undef TARGET_LEGITIMATE_COMBINED_INSN
> -#define TARGET_LEGITIMATE_COMBINED_INSN ix86_legitimate_combined_insn
> -
>  #undef TARGET_ASAN_SHADOW_OFFSET
>  #define TARGET_ASAN_SHADOW_OFFSET ix86_asan_shadow_offset
>
> --
> 2.5.5
>

On Tue, Apr 19, 2016 at 8:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Apr 19, 2016 at 4:49 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>
>> From INSTRUCTION EXCEPTION SPECIFICATION section in Intel software
>> developer manual volume 2, only legacy SSE instructions with memory
>> operand not 16-byte aligned get General Protection fault.  There is
>> no need to check 1, 2, 4, 8 byte alignments.  Since x86 backend has
>> accurate constraints and predicates for 16-byte alignment, we can
>> remove ix86_legitimate_combined_insn.
>>
>> Tested on x86-64.  OK for trunk?
>
> No. This function also handles cases where invalid hard register gets
> propagated into the insn during the combine pass, leading to spill
> failure later.
>

ix86_legitimate_combined_insn was added to work around the
reload issue:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46829
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46843

LRA doesn't have those limitation.  Removing
ix86_legitimate_combined_insn causes no regressions.

On Tue, Apr 19, 2016 at 5:18 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Apr 19, 2016 at 8:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Apr 19, 2016 at 4:49 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>>
>>> From INSTRUCTION EXCEPTION SPECIFICATION section in Intel software
>>> developer manual volume 2, only legacy SSE instructions with memory
>>> operand not 16-byte aligned get General Protection fault.  There is
>>> no need to check 1, 2, 4, 8 byte alignments.  Since x86 backend has
>>> accurate constraints and predicates for 16-byte alignment, we can
>>> remove ix86_legitimate_combined_insn.
>>>
>>> Tested on x86-64.  OK for trunk?
>>
>> No. This function also handles cases where invalid hard register gets
>> propagated into the insn during the combine pass, leading to spill
>> failure later.
>>
>
> ix86_legitimate_combined_insn was added to work around the
> reload issue:

Sorry, I'm not convinced. Please see [1].

You should remove only this part, together with now unused ssememalign
attribute.

-         /* For pre-AVX disallow unaligned loads/stores where the
-            instructions don't support it.  */
-         if (!TARGET_AVX
-             && VECTOR_MODE_P (mode)
-             && misaligned_operand (op, mode))
-           {
-             unsigned int min_align = get_attr_ssememalign (insn);
-             if (min_align == 0
-                 || MEM_ALIGN (op) < min_align)
-               return false;
-           }

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46829
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46843

> LRA doesn't have those limitation.  Removing
> ix86_legitimate_combined_insn causes no regressions.

[1] https://gcc.gnu.org/ml/gcc-patches/2012-08/msg01195.html

Uros.