On Fri, Mar 9, 2018 at 6:45 AM, Alexandre Oliva <aol...@redhat.com> wrote:
> LRA gets very confused when non-addresses are passed as operands to
> asms with address contraints. Even if other constraints are
> available, and the operand is a perfect fit for them, we'd still
> attempt to process the operand as an address, and fail miserably at
> that.
>
> Truth is, address constraints expect operands allowed by
> address_operand, and we make sure this is the case throughout the
> compiler, even in asm statements. The problem was that, if multiple
> constraints were available, we wouldn't insist that the operand be
> allowed by address_operand, but we would proceed as if it was,
> regardless of any other constraints.
>
> To address this problem, I've arranged for LRA to attempt to deal with
> address-constrained operands as addresses only when the is_address
> flag is set, and to not set this flag in preprocess_constraints for
> asm operands that are not allowed by address_operand.
>
> Regstrapped on i686- and x86_64-linux-gnu. Ok to install?
>
> for gcc/ChangeLog
>
> PR rtl-optimization/84682
> * lra-constraints.c (process_address_1): Check is_address flag
> for address constraints.
> (process_alt_operands): Likewise.
> * lra.c (lra_set_insn_recog_data): Pass asm operand locs to
> preprocess_constraints.
> * recog.h (preprocess_constraints): Add oploc parameter.
> Adjust callers.
> * recog.c (preprocess_constraints): Test address_operand for
> CT_ADDRESS constraints.
>
> for gcc/testsuite/ChangeLog
>
> PR rtl-optimization/84682
> * gcc.dg/torture/pr84682-1.c: New.
> * gcc.dg/torture/pr84682-2.c: New.
Hi Alexandre,
This test failed because of ICE on various aarch64 targets with below
error message:
Executing on host: /.../build/build-aarch64-none-elf/obj/gcc2/gcc/xgcc
-B/.../build/build-aarch64-none-elf/obj/gcc2/gcc/
/.../build/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c
-fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -S
-specs=aem-ve.specs -mcmodel=small -o pr84682-2.s (timeout =
300)
spawn /.../build/build-aarch64-none-elf/obj/gcc2/gcc/xgcc
-B/.../build/build-aarch64-none-elf/obj/gcc2/gcc/
/.../build/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c
-fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -S
-specs=aem-ve.specs -mcmodel=small -o pr84682-2.s
during RTL pass: expand
/.../build/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c: In function 'b':
/.../build/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c:9:3:
internal compiler error: in aarch64_classify_address, at
config/aarch64/aarch64.c:5678
0xfe3c29 aarch64_classify_address
/.../build/src/gcc/gcc/config/aarch64/aarch64.c:5677
0xfe8be8 aarch64_legitimate_address_hook_p
/.../build/src/gcc/gcc/config/aarch64/aarch64.c:5958
0xc0149e default_addr_space_legitimate_address_p(machine_mode,
rtx_def*, bool, unsigned char)
/.../build/src/gcc/gcc/targhooks.c:1476
0xb5b9f1 memory_address_addr_space_p(machine_mode, rtx_def*, unsigned char)
/.../build/src/gcc/gcc/recog.c:1334
0xb5d278 address_operand(rtx_def*, machine_mode)
/.../build/src/gcc/gcc/recog.c:1073
0xb5e186 asm_operand_ok(rtx_def*, char const*, char const**)
/.../build/src/gcc/gcc/recog.c:1816
0x73f440 expand_asm_stmt
/.../build/src/gcc/gcc/cfgexpand.c:3138
0x742d3c expand_gimple_stmt_1
/.../build/src/gcc/gcc/cfgexpand.c:3621
0x742d3c expand_gimple_stmt
/.../build/src/gcc/gcc/cfgexpand.c:3790
0x745c15 expand_gimple_basic_block
/.../build/src/gcc/gcc/cfgexpand.c:5819
0x749c2f execute
/.../build/src/gcc/gcc/cfgexpand.c:6425
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
Not sure if it reveals latent bug or just inconsistent issue with
backend though.
Thanks,
bin
> * gcc.dg/torture/pr84682-3.c: New.
> ---
> gcc/lra-constraints.c | 15 ++++++++++++++-
> gcc/lra.c | 3 ++-
> gcc/recog.c | 24 +++++++++++++++++-------
> gcc/recog.h | 2 +-
> gcc/testsuite/gcc.dg/torture/pr84682-1.c | 5 +++++
> gcc/testsuite/gcc.dg/torture/pr84682-2.c | 10 ++++++++++
> gcc/testsuite/gcc.dg/torture/pr84682-3.c | 8 ++++++++
> 7 files changed, 57 insertions(+), 10 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr84682-1.c
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr84682-2.c
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr84682-3.c
>
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index 59b97540d98f..ae55c86f1777 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -2276,6 +2276,12 @@ process_alt_operands (int only_alternative)
> break;
>
> case CT_ADDRESS:
> + /* An asm operand with an address constraint
> + that doesn't satisfy address_operand has
> + is_address cleared, so that we don't try to
> + make a non-address fit. */
> + if (!curr_static_id->operand[nop].is_address)
> + break;
> /* If we didn't already win, we can reload the address
> into a base register. */
> if (satisfies_address_constraint_p (op, cn))
> @@ -3236,7 +3242,14 @@ process_address_1 (int nop, bool check_only_p,
> && GET_CODE (XEXP (op, 0)) == SCRATCH)
> return false;
>
> - if (insn_extra_address_constraint (cn))
> + if (insn_extra_address_constraint (cn)
> + /* When we find an asm operand with an address constraint that
> + doesn't satisfy address_operand to begin with, we clear
> + is_address, so that we don't try to make a non-address fit.
> + If the asm statement got this far, it's because other
> + constraints are available, and we'll use them, disregarding
> + the unsatisfiable address ones. */
> + && curr_static_id->operand[nop].is_address)
> decompose_lea_address (&ad, curr_id->operand_loc[nop]);
> /* Do not attempt to decompose arbitrary addresses generated by combine
> for asm operands with loose constraints, e.g 'X'. */
> diff --git a/gcc/lra.c b/gcc/lra.c
> index a64d8f1a3011..0a251144026c 100644
> --- a/gcc/lra.c
> +++ b/gcc/lra.c
> @@ -1039,7 +1039,8 @@ lra_set_insn_recog_data (rtx_insn *insn)
> {
> operand_alternative *op_alt = XCNEWVEC (operand_alternative,
> nalt * nop);
> - preprocess_constraints (nop, nalt, constraints, op_alt);
> + preprocess_constraints (nop, nalt, constraints, op_alt,
> + data->operand_loc);
> setup_operand_alternative (data, op_alt);
> }
> }
> diff --git a/gcc/recog.c b/gcc/recog.c
> index af6a6b01d88c..fa8e261d1476 100644
> --- a/gcc/recog.c
> +++ b/gcc/recog.c
> @@ -2331,15 +2331,20 @@ extract_insn (rtx_insn *insn)
> which_alternative = -1;
> }
>
> -/* Fill in OP_ALT_BASE for an instruction that has N_OPERANDS operands,
> - N_ALTERNATIVES alternatives and constraint strings CONSTRAINTS.
> - OP_ALT_BASE has N_ALTERNATIVES * N_OPERANDS entries and CONSTRAINTS
> - has N_OPERANDS entries. */
> +/* Fill in OP_ALT_BASE for an instruction that has N_OPERANDS
> + operands, N_ALTERNATIVES alternatives and constraint strings
> + CONSTRAINTS. OP_ALT_BASE has N_ALTERNATIVES * N_OPERANDS entries
> + and CONSTRAINTS has N_OPERANDS entries. OPLOC should be passed in
> + if the insn is an asm statement and preprocessing should take the
> + asm operands into account, e.g. to determine whether they could be
> + addresses in constraints that require addresses; it should then
> + point to an array of pointers to each operand. */
>
> void
> preprocess_constraints (int n_operands, int n_alternatives,
> const char **constraints,
> - operand_alternative *op_alt_base)
> + operand_alternative *op_alt_base,
> + rtx **oploc)
> {
> for (int i = 0; i < n_operands; i++)
> {
> @@ -2426,6 +2431,9 @@ preprocess_constraints (int n_operands, int
> n_alternatives,
> break;
>
> case CT_ADDRESS:
> + if (oploc && !address_operand (*oploc[i], VOIDmode))
> + break;
> +
> op_alt[i].is_address = 1;
> op_alt[i].cl
> = (reg_class_subunion
> @@ -2470,7 +2478,8 @@ preprocess_insn_constraints (unsigned int icode)
>
> for (int i = 0; i < n_operands; ++i)
> constraints[i] = insn_data[icode].operand[i].constraint;
> - preprocess_constraints (n_operands, n_alternatives, constraints, op_alt);
> + preprocess_constraints (n_operands, n_alternatives, constraints, op_alt,
> + NULL);
>
> this_target_recog->x_op_alt[icode] = op_alt;
> return op_alt;
> @@ -2493,7 +2502,8 @@ preprocess_constraints (rtx_insn *insn)
> int n_entries = n_operands * n_alternatives;
> memset (asm_op_alt, 0, n_entries * sizeof (operand_alternative));
> preprocess_constraints (n_operands, n_alternatives,
> - recog_data.constraints, asm_op_alt);
> + recog_data.constraints, asm_op_alt,
> + NULL);
> recog_op_alt = asm_op_alt;
> }
> }
> diff --git a/gcc/recog.h b/gcc/recog.h
> index 4a47ff23ca95..eca62803458c 100644
> --- a/gcc/recog.h
> +++ b/gcc/recog.h
> @@ -136,7 +136,7 @@ extern void extract_constrain_insn (rtx_insn *insn);
> extern void extract_constrain_insn_cached (rtx_insn *);
> extern void extract_insn_cached (rtx_insn *);
> extern void preprocess_constraints (int, int, const char **,
> - operand_alternative *);
> + operand_alternative *, rtx **);
> extern const operand_alternative *preprocess_insn_constraints (unsigned int);
> extern void preprocess_constraints (rtx_insn *);
> extern rtx_insn *peep2_next_insn (int);
> diff --git a/gcc/testsuite/gcc.dg/torture/pr84682-1.c
> b/gcc/testsuite/gcc.dg/torture/pr84682-1.c
> new file mode 100644
> index 000000000000..b189ed78cdc3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr84682-1.c
> @@ -0,0 +1,5 @@
> +/* { dg-do compile } */
> +
> +void b(char a) {
> + asm("" : : "pir" (a));
> +}
> diff --git a/gcc/testsuite/gcc.dg/torture/pr84682-2.c
> b/gcc/testsuite/gcc.dg/torture/pr84682-2.c
> new file mode 100644
> index 000000000000..5abda5fd136c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr84682-2.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +
> +int a;
> +void b() {
> + float c;
> + for (int d; d;)
> + ;
> + a = c;
> + asm("" : : "pir"(c));
> +}
> diff --git a/gcc/testsuite/gcc.dg/torture/pr84682-3.c
> b/gcc/testsuite/gcc.dg/torture/pr84682-3.c
> new file mode 100644
> index 000000000000..543a307d6c12
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr84682-3.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* This is like pr84682-1.c, but with an extra memory constraint, to
> + check that we don't disable process_address altogether just because
> + of the disabled address constraint. */
> +
> +void b(char a) {
> + asm("" : : "pmir" (a));
> +}
>
>
> --
> Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/ FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
