On 01/28/2016 08:05 AM, Jakub Jelinek wrote:
On Wed, Jan 27, 2016 at 04:01:23PM -0500, Vladimir Makarov wrote:
The following patch fixes PR69299.
The details of the problem is described on
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69299
The patch was successfully bootstrapped and tested on x86/x86-64.
The patch introduces a new type of constraints
define_special_memory_constraint for memory constraints whose address reload
can not make memory to satisfy the constraint. It is useful when
specifically aligned memory is necessary or desirable. I don't know what is
the best name for this constraint. I use special_memory_constraint but it
could be more specific, e.g. aligned_memory_constraint. Please let me know
what is the best name for you.
Is the patch ok to commit?
I support the general idea and for naming will defer to Jeff, just have some
nits below. But it is your code and area of expertise, so I don't claim to
be an expert here...
Thanks for the comments, Jakub. I am not sure about all these changes
-- tt is hard to predict all possible consequences of the decisions
because of the RA complexity. But some comments made me rethink the patch.
--- ira-costs.c (revision 232571)
+++ ira-costs.c (working copy)
@@ -777,6 +777,7 @@ record_reg_classes (int n_alts, int n_op
break;
case CT_MEMORY:
+ case CT_SPECIAL_MEMORY:
/* Every MEM can be reloaded to fit. */
insn_allows_mem[i] = allows_mem[i] = 1;
if (MEM_P (op))
The comment is true only for CT_MEMORY. Wonder if it wouldn't be better to
handle CT_SPECIAL_MEMORY separately, perhaps as:
case CT_SPECIAL_MEMORY:
if (MEM_P (op) && constraint_satisfied_p (op, cn))
{
insn_allows_mem[i] = allows_mem[i] = 1;
win = 1;
}
break;
? I.e. if the constraint is already satisfied, treat it like memory
constraint, otherwise treat like (unsatisfied) fixed form constraint.
Or, if you want to account for the possibility that it doesn't satisfy the
constraint yet due to address that if reloaded would make it satisfy,
consider !memory_operand (op, ...) case as unknown, with no need to
check the constraint. Because if op satisfies already memory_operand,
but doesn't constraint_satisfied_p, it means it will never satisfy.
The difference in code most probably does not affect generated code
correctness but may change code quality. After some thinking, I decided
to change it to
case CT_SPECIAL_MEMORY:
if (MEM_P (op) && constraint_satisfied_p (op, cn))
win = 1;
allows_mem[i] = insn_allows_mem[i] = 1;
break;
--- ira.c (revision 232571)
+++ ira.c (working copy)
@@ -1868,6 +1868,7 @@ ira_setup_alts (rtx_insn *insn, HARD_REG
case CT_ADDRESS:
case CT_MEMORY:
+ case CT_SPECIAL_MEMORY:
goto op_success;
Perhaps treat it like CT_FIXED_FORM here instead? I.e. op_success only
if the constraint is satisfied? Or perhaps treat it as possible op_success
if !memory_operand, as mentioned above.
This would exclude consideration of possible transformation: unaligned
memory -> reg -> aligned memory. LRA can figure out the profitability
of this. But taking complexity of RA we can do unaligned memory ->
pseudo first assigning a hard reg to the pseudo and on later subpasses
to spill the pseudo for some reasons. Removing such RA freedom might
result in having LRA stuck.
--- recog.c (revision 232571)
+++ recog.c (working copy)
@@ -1791,6 +1791,7 @@ asm_operand_ok (rtx op, const char *cons
break;
case CT_MEMORY:
+ case CT_SPECIAL_MEMORY:
/* Every memory operand can be reloaded to fit. */
result = result || memory_operand (op, VOIDmode);
break;
Again, I'd treat it like CT_FIXED_FORM here, or at least if
memory_operand check also constraint_satisfied_p.
I would not change it. The reasons are described in the previous comment.
I'll test the change for ira-costs.c. If I find something wrong I'll
wrote about it.
