A potential bug in lra-constraints.c for special_memory_constraint?

A potential bug in lra-constraints.c for special_memory_constraint?

[Qing Zhao]

Hi,team:


The following doc for memory_constraint and special_memory_constraint seems
imply that the handling of the special_memory_constraint in lra-constraints.c
is NOT correct:

(https://gcc.gnu.org/onlinedocs/gccint/Define-Constraints.html#Define-Constraints)

MD Expression: define_memory_constraint name docstring exp

    Use this expression for constraints that match a subset of all memory
operands: that is, reload can make them match by converting the operand to
the form `(mem (reg X))¿, where X is a base register (from the register class
specified by BASE_REG_CLASS, see Register Classes).

….

    The syntax and semantics are otherwise identical to define_constraint.


MD Expression: define_special_memory_constraint name docstring exp

    Use this expression for constraints that match a subset of all memory
operands: that is, reload can not make them match by reloading the address as
it is described for define_memory_constraint or such address reload is
undesirable with the performance point of view.

    For example, define_special_memory_constraint can be useful if
specifically aligned memory is necessary or desirable for some insn operand.

    The syntax and semantics are otherwise identical to define_constraint.


From the above doc, the major difference between a memory_constraint and a
special_memory_constraint is:  whether "reload can or cannot make them match
by reloading the address".

For memory_constraint,  the reload is Okay, however, for
special_memory_constraint, the reload is NOT Okay.

I am not sure whether the RELOAD includes Spill or not, if it is, then the
current handling of special_memory_constraint is NOT correct:
(lra-constraints.c)

2088                     case CT_SPECIAL_MEMORY:
2089                       if (MEM_P (op)
2090                           && satisfies_memory_constraint_p (op, cn))
2091                         win = true;
2092                       else if (spilled_pseudo_p (op))
2093                         win = true;
2094                       break;

line 2092-2093 permits the memory spill, which seems need to be avoided for
SPECIAL_MEMORY_Constraint.

the thing I need to confirm is:

whether “spill” is considered as RELOAD or NOT?

if the spill IS RELOAD, then the handling of special_memory_constraint in lra-constraints.c is 
definitely wrong, we should fix this issue in upstream. 

thanks.

Qing

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Eric Botcazou]

> From the above doc, the major difference between a memory_constraint and a
> special_memory_constraint is:  whether "reload can or cannot make them match
> by reloading the address".

Right, i.e. by just changing the form of the address (instead of the address 
itself).

> For memory_constraint,  the reload is Okay, however, for
> special_memory_constraint, the reload is NOT Okay.
> 
> I am not sure whether the RELOAD includes Spill or not, if it is, then the
> current handling of special_memory_constraint is NOT correct:
> (lra-constraints.c)
> 
> 2088                     case CT_SPECIAL_MEMORY:
> 2089                       if (MEM_P (op)
> 2090                           && satisfies_memory_constraint_p (op, cn))
> 2091                         win = true;
> 2092                       else if (spilled_pseudo_p (op))
> 2093                         win = true;
> 2094                       break;
> 
> line 2092-2093 permits the memory spill, which seems need to be avoided for
> SPECIAL_MEMORY_Constraint.
> 
> the thing I need to confirm is:
> 
> whether “spill” is considered as RELOAD or NOT?

Yes, spilling is part of reloading but is not reloading the address since it's 
changing the address, so I think it's OK.  Why is that problematic for you?

-- 
Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Qing Zhao]

thanks for the replying.

> On Jul 11, 2017, at 2:44 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> From the above doc, the major difference between a memory_constraint and a
>> special_memory_constraint is:  whether "reload can or cannot make them match
>> by reloading the address".
> 
> Right, i.e. by just changing the form of the address (instead of the address 
> itself).
> 
>> For memory_constraint,  the reload is Okay, however, for
>> special_memory_constraint, the reload is NOT Okay.
>> 
>> I am not sure whether the RELOAD includes Spill or not, if it is, then the
>> current handling of special_memory_constraint is NOT correct:
>> (lra-constraints.c)
>> 
>> 2088                     case CT_SPECIAL_MEMORY:
>> 2089                       if (MEM_P (op)
>> 2090                           && satisfies_memory_constraint_p (op, cn))
>> 2091                         win = true;
>> 2092                       else if (spilled_pseudo_p (op))
>> 2093                         win = true;
>> 2094                       break;
>> 
>> line 2092-2093 permits the memory spill, which seems need to be avoided for
>> SPECIAL_MEMORY_Constraint.
>> 
>> the thing I need to confirm is:
>> 
>> whether “spill” is considered as RELOAD or NOT?
> 
> Yes, spilling is part of reloading but is not reloading the address since it's 
> changing the address, so I think it's OK.  Why is that problematic for you?

the problem I had is:

1. we added a new special_memory_constraint for misaligned memory access,  one important requirement
for this new special_memory_constraint is, the address of the memory access is misaligned.

2. per the current code in lra-constraints.c:

2286                     case CT_SPECIAL_MEMORY:
2287                       if (MEM_P (op)
2288                           && satisfies_memory_constraint_p (op, cn))
2289                         win = true;
2290                       else if (spilled_pseudo_p (op))
2291                         win = true;
2292                       break;

if the op is a pseudo_p that can be spilled, then it's treated as a
PERFECT MATCH.

the issue only can be exposed by the following kind of RTL:

(insn 34 13 14 2 (set (reg:DI 122)
       (reg:DI 129)) misalign-3.c:12 125 {*movdi_insn_sp64}
    (nil))

i.e.
        (1). REG2 move to REG1
and. (2). REG2 is a virtual reg (> the max hard regno, on Sparc, its 103),
therefore, must be spilled to stack.

the current interpretation of special memory treat such REG2 as a perfect
match to special memory,  and then spill it.
however, such spilled memory RTL is NOT match the MISALIGN requirement, (i.e, the address of the memory
access for the spilled RTL is not misaligned)

therefore triggered the failure later.

That’s the reason I tracked down to this potential issue in the handling of “CT_SPECIAL_MEMORY”.

thanks a lot.

Qing


> -- 
> Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Eric Botcazou]

> the problem I had is:
> 
> 1. we added a new special_memory_constraint for misaligned memory access, 
> one important requirement for this new special_memory_constraint is, the
> address of the memory access is misaligned.

OK, it's the other way around from the usage of special_memory_constraint.
In other words, I'm not sure that you really need to use it in this case,
a standard memory_constraint could be sufficient.

> 2. per the current code in lra-constraints.c:
> 
> 2286                     case CT_SPECIAL_MEMORY:
> 2287                       if (MEM_P (op)
> 2288                           && satisfies_memory_constraint_p (op, cn))
> 2289                         win = true;
> 2290                       else if (spilled_pseudo_p (op))
> 2291                         win = true;
> 2292                       break;
> 
> if the op is a pseudo_p that can be spilled, then it's treated as a
> PERFECT MATCH.
> 
> the issue only can be exposed by the following kind of RTL:
> 
> (insn 34 13 14 2 (set (reg:DI 122)
>        (reg:DI 129)) misalign-3.c:12 125 {*movdi_insn_sp64}
>     (nil))
> 
> i.e.
>         (1). REG2 move to REG1
> and. (2). REG2 is a virtual reg (> the max hard regno, on Sparc, its 103),
> therefore, must be spilled to stack.
> 
> the current interpretation of special memory treat such REG2 as a perfect
> match to special memory,  and then spill it.
> however, such spilled memory RTL is NOT match the MISALIGN requirement,
> (i.e, the address of the memory access for the spilled RTL is not
> misaligned)

Yes, spilling will automatically meet alignment requirements, that's why it's 
allowed for special_memory_constraint.

Why do you absolutely need to have a misaligned address?  Can't you just avert 
your eyes and pretend that the address is misaligned?  This will be suboptimal 
but presumably work.  To be honest, I'm not even sure that you really need an 
additional constraint, but I haven't investigated the subject seriously.

-- 
Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Qing Zhao]

> On Jul 11, 2017, at 2:00 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> the problem I had is:
>> 
>> 1. we added a new special_memory_constraint for misaligned memory access, 
>> one important requirement for this new special_memory_constraint is, the
>> address of the memory access is misaligned.
> 
> OK, it's the other way around from the usage of special_memory_constraint.
> In other words, I'm not sure that you really need to use it in this case,
> a standard memory_constraint could be sufficient.

A standard memory_constraint has the same handling on spilled code as special_memory_constraint:

2250                     case CT_MEMORY:
2251                       if (MEM_P (op)
2252                           && satisfies_memory_constraint_p (op, cn))
2253                         win = true;
2254                       else if (spilled_pseudo_p (op))
2255                         win = true;

as a result, if the new misaligned memory access was defined as a standard memory_constraint, will have the same issue.


> 
>> 2. per the current code in lra-constraints.c:
>> 
>> 2286                     case CT_SPECIAL_MEMORY:
>> 2287                       if (MEM_P (op)
>> 2288                           && satisfies_memory_constraint_p (op, cn))
>> 2289                         win = true;
>> 2290                       else if (spilled_pseudo_p (op))
>> 2291                         win = true;
>> 2292                       break;
>> 
>> if the op is a pseudo_p that can be spilled, then it's treated as a
>> PERFECT MATCH.
>> 
>> the issue only can be exposed by the following kind of RTL:
>> 
>> (insn 34 13 14 2 (set (reg:DI 122)
>>       (reg:DI 129)) misalign-3.c:12 125 {*movdi_insn_sp64}
>>    (nil))
>> 
>> i.e.
>>        (1). REG2 move to REG1
>> and. (2). REG2 is a virtual reg (> the max hard regno, on Sparc, its 103),
>> therefore, must be spilled to stack.
>> 
>> the current interpretation of special memory treat such REG2 as a perfect
>> match to special memory,  and then spill it.
>> however, such spilled memory RTL is NOT match the MISALIGN requirement,
>> (i.e, the address of the memory access for the spilled RTL is not
>> misaligned)
> 
> Yes, spilling will automatically meet alignment requirements, that's why it's 
> allowed for special_memory_constraint.

You mean, even for the mis-alignment requirement, the spilled memory access will met the mis-alignment?

> 
> Why do you absolutely need to have a misaligned address?
we need to generate misaligned load/store insns ONLY for misaligned memory access, therefore need a new 
constraints for misaligned address.

As I checked the GCC source code, the special_memory_constraints only were defined in i386 and sparc target, not
used quite often.

Qing

>  Can't you just avert 
> your eyes and pretend that the address is misaligned?  This will be suboptimal 
> but presumably work.  To be honest, I'm not even sure that you really need an 
> additional constraint, but I haven't investigated the subject seriously.
> 
> -- 
> Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Eric Botcazou]

> we need to generate misaligned load/store insns ONLY for misaligned memory
> access, therefore need a new constraints for misaligned address.

Why?  What happens exactly if the memory access turns out to be aligned?

-- 
Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Qing Zhao]

> On Jul 11, 2017, at 4:24 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> we need to generate misaligned load/store insns ONLY for misaligned memory
>> access, therefore need a new constraints for misaligned address.
> 
> Why?  What happens exactly if the memory access turns out to be aligned?

we add this new constraint as:

;; We need a special memory constraint for the misaligned memory access
;; This is only for TARGET_MISALIGN target
(define_special_memory_constraint "B"
 "Memory reference whose address is misaligned"
 (and (match_code "mem")
      (match_test "TARGET_MISALIGN")
      (match_test "memory_is_misaligned (op, mode)”)))

the routine “memory_is_misaligned” is a compile-time check to see whether the address is known to be
misaligned or not. only for compile-time KNOWN misaligned memory access, we will use misaligned load/store insns
provided by the new processor for the memory access. 

and then put this new constraints to sparc.md as: 

(define_insn "*movdi_insn_sp64"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,r, B, m, r,*e,?*e,?*e,?W,b,b")
        (match_operand:DI 1 "input_operand"        "rI,N,B,m,rJ,rJ,*e, r, *e,  W,*e,J,P"))]


NOTE, the 4th constraints for this insn is “B, rJ”,  if the operands match this constraint, then.  misaligned store insns will be generated for the
misaligned memory access instead of regular store. 

misaligned insns will NOT be used for memory access whose alignment cannot be decided to be misaligned during compilation time.

Hope this is clear.  If I still don’t answer your question, please let me know.

Qing
 

> 
> -- 
> Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Eric Botcazou]

> we add this new constraint as:
> 
> ;; We need a special memory constraint for the misaligned memory access
> ;; This is only for TARGET_MISALIGN target
> (define_special_memory_constraint "B"
>  "Memory reference whose address is misaligned"
>  (and (match_code "mem")
>       (match_test "TARGET_MISALIGN")
>       (match_test "memory_is_misaligned (op, mode)”)))
> 
> the routine “memory_is_misaligned” is a compile-time check to see whether
> the address is known to be misaligned or not. only for compile-time KNOWN
> misaligned memory access, we will use misaligned load/store insns provided
> by the new processor for the memory access.
> 
> and then put this new constraints to sparc.md as:
> 
> (define_insn "*movdi_insn_sp64"
>   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,r, B, m,
> r,*e,?*e,?*e,?W,b,b") (match_operand:DI 1 "input_operand"       
> "rI,N,B,m,rJ,rJ,*e, r, *e,  W,*e,J,P"))]
> 
> 
> NOTE, the 4th constraints for this insn is “B, rJ”,  if the operands match
> this constraint, then.  misaligned store insns will be generated for the
> misaligned memory access instead of regular store.

OK, but what happens in the end?  What's the failure mode?  Internal compiler 
error, impossible reloading, wrong code, suboptimal code, etc?

-- 
Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Wed, 12 Jul 2017 01:19:03 +0200

>> we add this new constraint as:
>> 
>> ;; We need a special memory constraint for the misaligned memory access
>> ;; This is only for TARGET_MISALIGN target
>> (define_special_memory_constraint "B"
>>  "Memory reference whose address is misaligned"
>>  (and (match_code "mem")
>>       (match_test "TARGET_MISALIGN")
>>       (match_test "memory_is_misaligned (op, mode)”)))
>> 
>> the routine “memory_is_misaligned” is a compile-time check to see whether
>> the address is known to be misaligned or not. only for compile-time KNOWN
>> misaligned memory access, we will use misaligned load/store insns provided
>> by the new processor for the memory access.
>> 
>> and then put this new constraints to sparc.md as:
>> 
>> (define_insn "*movdi_insn_sp64"
>>   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,r, B, m,
>> r,*e,?*e,?*e,?W,b,b") (match_operand:DI 1 "input_operand"       
>> "rI,N,B,m,rJ,rJ,*e, r, *e,  W,*e,J,P"))]
>> 
>> 
>> NOTE, the 4th constraints for this insn is “B, rJ”,  if the operands match
>> this constraint, then.  misaligned store insns will be generated for the
>> misaligned memory access instead of regular store.
> 
> OK, but what happens in the end?  What's the failure mode?  Internal compiler 
> error, impossible reloading, wrong code, suboptimal code, etc?

Yeah I'm still hopelessly confused what the problem is too.

As far as I understand it, the unaligned loads and stores present in
the M8 can be used just fine on aligned data.  It's just not efficient
(11 cycle latency instead of 3).

Perhaps they are just trying to only match the constraints for the
the unaligned loads and stores when absolutely necessary.

If so, I really really wish they had said this from the beginning. :)

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Eric Botcazou]

> Actually, My major question is whether the current handling of
> special_memory_constraint in lra_constraints.c is correct or NOT based on
> GCC internal documentation. I thought that’s independent from this
> misaligned insns generation for M8, but looks I was wrong.

The answer is yes, the current handling is definitely correct, which probably 
means that a special_memory_constraint ought not to be used here and that a 
usual memory_constraint should work just fine.

-- 
Eric Botcazou

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[David Miller]

From: Qing Zhao <qing.zhao@oracle.com>
Date: Wed, 12 Jul 2017 08:49:52 -0500

> and it also clearly mentioned that “specially aligned memory might
> use this constraint”.

It guarantees the achieve the opposite of what you are trying to do.

That is, it can be used to guarantee that something is aligned to a
multiple of X or greater.

What you want is to know that something is guaranteed to be
aligned less strongly that X.  And that invariant is not
provided for.

Re: A potential bug in lra-constraints.c for special_memory_constraint?

[Qing Zhao]

> On Jul 12, 2017, at 10:28 AM, David Miller <davem@davemloft.net> wrote:
> 
> From: Qing Zhao <qing.zhao@oracle.com>
> Date: Wed, 12 Jul 2017 08:49:52 -0500
> 
>> and it also clearly mentioned that “specially aligned memory might
>> use this constraint”.
> 
> It guarantees the achieve the opposite of what you are trying to do.
> 
> That is, it can be used to guarantee that something is aligned to a
> multiple of X or greater.

so, the spill code can guarantee to provide a special alignment for multiple of X or greater?


> 
> What you want is to know that something is guaranteed to be
> aligned less strongly that X.  And that invariant is not
> provided for.

are you implying that special_memory_constraint is NOT for my current purpose?

thanks.

Qing