Vladimir Makarov <[email protected]> writes:
> On 2021-03-19 11:42 a.m., Richard Sandiford wrote:
>> Vladimir Makarov via Gcc-patches <[email protected]> writes:
>>> The following patch solves P1 PR99581
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99581
>>>
>>> The patch was successfully tested and bootstrapped on x86-64, ppc64le,
>>> aarch64.
>>>
>>> Is it ok for the trunk?
>>
>> I'm not trying to reject the patch as such. I just think we need to
>> have a clearer picture first.
>>
> I agree that 'o' should be treated as a subset of 'm' and therefore its
> definition should have a check as 'm' has. Still my patch is not about
> treatment of constraint 'o' only.
>
> My approach for LRA development is minimal changes, as radical changes
> (even if they look right) results long lasting unpredictable effects on
> many targets.
>
> The patch in which you introduced a new function valid_address_p and new
> treatment of **all** memory constraints was too big change with this
> point of view and finally it resulted in this problem after recent
> partially fixing mess in process_address_1.
>
> My patch fixes this radical change. So I think we still need the patch
> I've submitted.
OK, fair enough. I have some minor cosmetic comments below, but
otherwise the patch is OK for trunk and branch.
Thanks,
Richard
> diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
> index e3686dbfe61..90dd0401fa0 100644
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -4567,6 +4567,17 @@ The syntax and semantics are otherwise identical to
> @code{define_constraint}.
> @end deffn
> +@deffn {MD Expression} define_relaxed_memory_constraint name docstring exp
> +Usually memory address in @code{reload} pass is checked to be a legitimate
> +one besides checking the memory itself to satisfy the instruction
> +constraint. Sometimes you need to avoid legitimate address check for
> +memory and use only check for memory to satisfy the constraint. Use
> +this expression to describe operands for such cases.
How about something like:
-----------------------------------------------------------------------
The test expression in a @code{define_memory_constraint} can assume
that @code{TARGET_LEGITIMATE_ADDRESS_P} holds for the address inside
a @code{mem} rtx and so it does not need to test this condition itself.
In other words, a @code{define_memory_constraint} test of the form:
@smallexample
(match_test "mem")
@end smallexample
is enough to test whether an rtx is a @code{mem} @emph{and} whether
its address satisfies @code{TARGET_MEM_CONSTRAINT} (which is usually
@samp{'m'}). Thus the conditions imposed by a @code{define_memory_constraint}
always apply on top of the conditions imposed by @code{TARGET_MEM_CONSTRAINT}.
However, it is sometimes useful to define memory constraints that allow
addresses beyond those accepted by @code{TARGET_LEGITIMATE_ADDRESS_P}.
@code{define_relaxed_memory_constraint} exists for this case.
The test expression in a @code{define_relaxed_memory_constraint} is
applied with no preconditions, so that the expression can determine
``from scratch'' exactly which addresses are valid and which are not.
-----------------------------------------------------------------------
> +
> +The syntax and semantics are otherwise identical to
> +@code{define_constraint}.
Think @code{define_memory_constraint} would be a better
reference point here.
> @@ -756,15 +759,15 @@ mangle (const char *name)
> return XOBFINISH (rtl_obstack, const char *);
> }
> -/* Add one constraint, of any sort, to the tables. NAME is its name;
> - REGCLASS is the register class, if any; EXP is the expression to
> - test, if any; IS_MEMORY, IS_SPECIAL_MEMORY and IS_ADDRESS indicate
> - memory, special memory, and address constraints, respectively; LOC
> - is the .md file location.
> +/* Add one constraint, of any sort, to the tables. NAME is its name;
> REGCLASS
> + is the register class, if any; EXP is the expression to test, if any;
> + IS_MEMORY, IS_SPECIAL_MEMORY, IS_RELAXED_MEMORY and IS_ADDRESS indicate
> + memory, special memory, and address constraints, respectively; LOC is the
> .md
Long line.
> + file location.
> Not all combinations of arguments are valid; most importantly,
> REGCLASS is mutually exclusive with EXP, and
> - IS_MEMORY/IS_SPECIAL_MEMORY/IS_ADDRESS are only meaningful for
> + IS_MEMORY/IS_SPECIAL_MEMORY/IS_RELAXED_MEMORY/IS_ADDRESS are only
> meaningful for
Same here.
> constraints with EXP.
> This function enforces all syntactic and semantic rules about what
> @@ -773,7 +776,7 @@ mangle (const char *name)
> static void
> add_constraint (const char *name, const char *regclass,
> rtx exp, bool is_memory, bool is_special_memory,
> - bool is_address, file_location loc)
> + bool is_relaxed_memory, bool is_address, file_location loc)
> {
> class constraint_data *c, **iter, **slot;
> const char *p;
> @@ -895,6 +898,17 @@ add_constraint (const char *name, const char *regclass,
> name, name[0]);
> return;
> }
> + else if (is_relaxed_memory)
> + {
> + if (name[1] == '\0')
> + error_at (loc, "constraint letter '%c' cannot be a "
> + "relaxed memory constraint", name[0]);
> + else
> + error_at (loc, "constraint name '%s' begins with '%c', "
> + "and therefore cannot be a relaxed memory constraint",
> + name, name[0]);
> + return;
> + }
I think we should just add “ || is_relaxed_memory” to the existing
“is_memory” condition (and should probably have done that with
is_special_memory). We aren't making a distinction between
different types of memory constraint in this context; we're just
making a distinction between constant and non-constant constraints.
> @@ -1529,6 +1555,8 @@ write_tm_preds_h (void)
> values.safe_push (std::make_pair (memory_start, "CT_MEMORY"));
> if (special_memory_start != special_memory_end)
> values.safe_push (std::make_pair (special_memory_start,
> "CT_SPECIAL_MEMORY"));
> + if (relaxed_memory_start != relaxed_memory_end)
> + values.safe_push (std::make_pair (relaxed_memory_start,
> "CT_RELAXED_MEMORY"));
Long line.
> if (address_start != address_end)
> values.safe_push (std::make_pair (address_start, "CT_ADDRESS"));
> if (address_end != num_constraints)
