Bug 65689 - [5 Regression][AArch64] S constraint fails for inline asm at -O0
Summary: [5 Regression][AArch64] S constraint fails for inline asm at -O0
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 5.0
: P2 minor
Target Milestone: 5.4
Assignee: Not yet assigned to anyone
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2015-04-07 17:29 UTC by Alan Lawrence
Modified: 2016-01-12 13:56 UTC (History)
2 users (show)

See Also:
Host:
Target: aarch64*-*-*
Build:
Known to work: 4.9.3
Known to fail: 5.0
Last reconfirmed: 2015-04-07 00:00:00


Attachments
gcc5-pr65689.patch (2.48 KB, patch)
2015-04-08 12:35 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Lawrence 2015-04-07 17:29:11 UTC
Starting with r221532 (https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01064.html),

void
test (void)
{
    __asm__ ("@ %c0" : : "S" (&test + 4));
}

fails to compile at -O0 on all aarch64 targets with:

c-output-template-3.c: In function 'test':
c-output-template-3.c:7:5: error: impossible constraint in 'asm'
     __asm__ ("@ %c0" : : "S" (&test + 4));

(This is gcc.target/aarch64/c-output-template-3.c, without the -O added in r221905, as that leads to successful compilation - however, the testcase should compile without -O too.)
Comment 1 Alan Lawrence 2015-04-07 17:32:54 UTC
Problem stems from parse_input_constraint (in stmt.c):

	if (reg_class_for_constraint (cn) != NO_REGS
	    || insn_extra_address_constraint (cn))
	  *allows_reg = true;
	else if (insn_extra_memory_constraint (cn))
	  *allows_mem = true;
	else
	  {
	    /* Otherwise we can't assume anything about the nature of
	       the constraint except that it isn't purely registers.
	       Treat it like "g" and hope for the best.  */
	    *allows_reg = true;
	    *allows_mem = true;
	  }

which causes expand_asm_operands to use (reg/f:DI ...), which fails the definition of the S constraint. If instead parse_input_constraint set both allows_reg and allows_mem to false (as it does for e.g. an "i" constraint, via a special-case), expand_asm_operands would follow the register to its definition:

(const:DI (plus:DI (symbol_ref:DI ("test") [flags 0x3] <function_decl 0x7fb7c60300 test>)
        (const_int 4 [0x4])))

(as also happens with -O), which satisfies the S constraint.

One solution could be to generalize the special case in parse_input_constraint.
Comment 2 James Greenhalgh 2015-04-07 20:16:15 UTC
Filling in the blank fields, and confirmed.
Comment 3 Jakub Jelinek 2015-04-08 09:54:06 UTC
I don't see why this should compile with -O0 actually, it assumes optimization being performed at -O0.
Comment 4 Jakub Jelinek 2015-04-08 11:01:54 UTC
Adding a hack where stmt.c (parse_input_constraint) so that it handles
case 'S':
the same as say case 'i':
fixes the test, so it is all about whether the expander and middle-end thinks that "S" constraint allows_reg || allows_mem (right now both, ideally none).
So, to fix this, perhaps hack up genpreds.c, so that it generates another function or two which returns true if the constraint can be easily proved not to allow reg (or not to allow mem).
Say "S" is:
  (and (match_code "const,symbol_ref,label_ref")
       (match_test "aarch64_symbolic_address_p (op)")))
and so genpreds can conservatively assume match_test can return anything, but
match_code clearly doesn't satisfy a reg/subreg nor mem (requires something other), so process_{input,output}_constraint should be able to safely set both of those to false.
Comment 5 Jakub Jelinek 2015-04-08 12:35:55 UTC
Created attachment 35258 [details]
gcc5-pr65689.patch

Untested fix.  For aarch64, there are lots of constraints determined by this patch to not allow either reg or mem:
S, Y, Ush, Uss, Usn, Usd, Usf, Ui3, Up3, Ufc, Dn, Dh, Dq, Dl, Dr, Dz, Dd
and then
V, <, >
determined as only *allows_mem.  But '<' and '>' are already handled in process_*_constraint earlier, so it is just V.  The rest are determined (conservatively) to allow either.
On x86_64, G is determined to allow neither, V, <, > similarly to aarch64 to only *allows_mem, the rest maybe both.
Comment 6 Alan Lawrence 2015-04-08 13:46:12 UTC
Whilst I think this probably would fix the problem - surely this will change the meaning of loads of constraints, on loads of platforms? I will of course defer to the release manager(s) (!), but IMHO this feels rather risky to do at this late stage, i.e. potentially "the cure is worse than the disease"...?

Secondly, do I understand correctly, that the constraint-parsing mechanism will only come into play for plain ol' define_constraints, whereeas define_register_constraint / define_memory_constraint would provide/override with their own values? Does this still leave us with consistent meaning for all three kinds of define...constraint?
Comment 7 Jakub Jelinek 2015-04-08 13:54:26 UTC
(In reply to alalaw01 from comment #6)
> Whilst I think this probably would fix the problem - surely this will change
> the meaning of loads of constraints, on loads of platforms? I will of course
> defer to the release manager(s) (!), but IMHO this feels rather risky to do
> at this late stage, i.e. potentially "the cure is worse than the disease"...?

The patch doesn't change meaning of any constraints, just attempts to provide more precise answers on what constraints allow registers and what allow memory.

> Secondly, do I understand correctly, that the constraint-parsing mechanism
> will only come into play for plain ol' define_constraints, whereeas
> define_register_constraint / define_memory_constraint would provide/override
> with their own values? Does this still leave us with consistent meaning for
> all three kinds of define...constraint?

The new generated function is only used for define_constraint, sure, define_register_constraint registered constraints are always handled as *allows_reg = true;, define_memory_constraint are always handled as *allows_mem = true;, define_address_constraint is handled also like *allows_reg = true;, all 3 already before and still after the patch unchanged.  Similarly most of the hardcoded well known architecture independent constraints.  The patch is only about the case where it previously just made a very conservative last bet, with the patch it still handles a couple of obvious constraints right and falls back just for the rest where even genpreds.c doesn't safely know.
Comment 8 Alan Lawrence 2015-04-09 08:57:01 UTC
Well, meaning/behaviour. But thanks for the patch - I've bootstrapped and check-gcc'd on AArch64 and arm hf (Cortex-A15 + Neon) with no regressions.
Comment 9 Jakub Jelinek 2015-04-17 16:44:00 UTC
Author: jakub
Date: Fri Apr 17 16:43:28 2015
New Revision: 222186

URL: https://gcc.gnu.org/viewcvs?rev=222186&root=gcc&view=rev
Log:
	PR target/65689
	* genpreds.c (struct constraint_data): Add maybe_allows_reg and
	maybe_allows_mem bitfields.
	(maybe_allows_none_start, maybe_allows_none_end,
	maybe_allows_reg_start, maybe_allows_reg_end, maybe_allows_mem_start,
	maybe_allows_mem_end): New variables.
	(compute_maybe_allows): New function.
	(add_constraint): Use it to initialize maybe_allows_reg and
	maybe_allows_mem fields.
	(choose_enum_order): Sort the non-is_register/is_const_int/is_memory/
	is_address constraints such that those that allow neither mem nor
	reg come first, then those that only allow reg but not mem, then
	those that only allow mem but not reg, then the rest.
	(write_allows_reg_mem_function): New function.
	(write_tm_preds_h): Call it.
	* stmt.c (parse_output_constraint, parse_input_constraint): Use
	the generated insn_extra_constraint_allows_reg_mem function
	instead of always setting *allows_reg = true; *allows_mem = true;
	for unknown extra constraints.

	* gcc.target/aarch64/c-output-template-4.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/aarch64/c-output-template-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/genpreds.c
    trunk/gcc/stmt.c
    trunk/gcc/testsuite/ChangeLog
Comment 10 Jakub Jelinek 2015-04-17 17:05:37 UTC
Should be fixed on the trunk now, queued for 5.2 if it is fine on the trunk.
Comment 11 Jakub Jelinek 2015-04-22 11:59:44 UTC
GCC 5.1 has been released.
Comment 12 Richard Biener 2015-07-16 09:14:03 UTC
GCC 5.2 is being released, adjusting target milestone to 5.3.
Comment 13 Richard Biener 2015-12-04 10:47:24 UTC
GCC 5.3 is being released, adjusting target milestone.
Comment 14 James Greenhalgh 2016-01-11 11:34:08 UTC
Jakub, were you planning to backport this fix to 5.4, or would you like me to do it?
Comment 15 Jakub Jelinek 2016-01-11 11:42:01 UTC
(In reply to James Greenhalgh from comment #14)
> Jakub, were you planning to backport this fix to 5.4, or would you like me
> to do it?

Forgot about this one.  If you could backport it, it would be greatly appreciated.
Comment 16 James Greenhalgh 2016-01-12 13:55:16 UTC
Author: jgreenhalgh
Date: Tue Jan 12 13:54:45 2016
New Revision: 232269

URL: https://gcc.gnu.org/viewcvs?rev=232269&root=gcc&view=rev
Log:
Backport: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

gcc/

	Backport from mainline r222186.
	2015-04-17  Jakub Jelinek  <jakub@redhat.com>

	PR target/65689
	* genpreds.c (struct constraint_data): Add maybe_allows_reg and
	maybe_allows_mem bitfields.
	(maybe_allows_none_start, maybe_allows_none_end,
	maybe_allows_reg_start, maybe_allows_reg_end, maybe_allows_mem_start,
	maybe_allows_mem_end): New variables.
	(compute_maybe_allows): New function.
	(add_constraint): Use it to initialize maybe_allows_reg and
	maybe_allows_mem fields.
	(choose_enum_order): Sort the non-is_register/is_const_int/is_memory/
	is_address constraints such that those that allow neither mem nor
	reg come first, then those that only allow reg but not mem, then
	those that only allow mem but not reg, then the rest.
	(write_allows_reg_mem_function): New function.
	(write_tm_preds_h): Call it.
	* stmt.c (parse_output_constraint, parse_input_constraint): Use
	the generated insn_extra_constraint_allows_reg_mem function
	instead of always setting *allows_reg = true; *allows_mem = true;
	for unknown extra constraints.

gcc/testsuite/

	Backport from mainline r222186.
	2015-04-17  Jakub Jelinek  <jakub@redhat.com>

	PR target/65689
	* gcc.target/aarch64/c-output-template-4.c: New test.


Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.target/aarch64/c-output-template-4.c
      - copied unchanged from r222186, trunk/gcc/testsuite/gcc.target/aarch64/c-output-template-4.c
Modified:
    branches/gcc-5-branch/   (props changed)
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/genpreds.c
    branches/gcc-5-branch/gcc/stmt.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog

Propchange: branches/gcc-5-branch/
            ('svn:mergeinfo' modified)
Comment 17 James Greenhalgh 2016-01-12 13:56:01 UTC
Should be fixed now.