[gcc] Use new constraint for LEA address operands.

[gcc] Use new constraint for LEA address operands.

The problem of processing LEA address operands is that it requires original, non-nacl address decomposition. Before this change, an original "p" constraint was used, and a global variable NACL_LEA_MATCH_ADDRESS_OPERAND signaled LEA decomposition mode to x86_decompose_address. That solution required appropriate setting of NACL_LEA_MATCH_ADDRESS_OPERAND before every call to constrain_operands.

Here I introduce new "T" constraint, and use custom decomposition function to process it. NACL_LEA_ADDRESS_OPERAND is still here, but it is not checked any more. Next change will cleanup it completely.

In fact, new address decomposition function is an original, non-nacl x86_decompose_address. For now, I just copy-paste it. Perhaps it is better to split x86_decompose_address into pieces and reuse them, but this better goes in a separate change.

[pasko-google - do not use, 2010-06-24 08:34:15]

LGTM

I really like the change! thanks! though I am not satisfied with my
understanding of the quirks that this change may introduce.

Overall I would like to put as much comments as necessary right now, not
postponing it to undefined dates. ChangeLog can be an exception from this rule.

Also, TODOs really confuse me, I don't understand their scope. See below.

http://codereview.chromium.org/2873014/diff/1/3
File gcc/gcc/config/i386/i386.c (left):

http://codereview.chromium.org/2873014/diff/1/3#oldcode8792
gcc/gcc/config/i386/i386.c:8792: /* Extract the parts of an RTL expression that
is a valid memory address
this text comments ix86_decompose_address, but for historical reasons it now
prepends lea_match_address_operand, which is wrong. Please, fix this also.

http://codereview.chromium.org/2873014/diff/1/3
File gcc/gcc/config/i386/i386.c (right):

http://codereview.chromium.org/2873014/diff/1/3#newcode8822
gcc/gcc/config/i386/i386.c:8822: +	     TODO: what if (plus (plus a b) (plus c
d)) ?  */
another idea: s/TODO/REVIEW_QUESTION/

http://codereview.chromium.org/2873014/diff/1/3#newcode11917
gcc/gcc/config/i386/i386.c:11917: +print_operand_address_parts (FILE *file,
const struct ix86_address *parts)
thanks for this! a comment would be nice though

http://codereview.chromium.org/2873014/diff/1/3#newcode19595
gcc/gcc/config/i386/i386.c:19595: +         TODO: factor out
memory_address_parts_length and call it here with
A general question: who is going to fix this TODO and when?  The comment
suggests some easy refactoring operation, then it's a good candidate to be done
now. If the situation is opposite, i.e. it's hard to fix, then, as usual, it'll
never be fixed.

Please, identify your nickname as a TODO author or eliminate the TODO either
way.

Also, I am pretty sure it's safe to return 0 because size computations are used
only for performance reasons. We are not precise about the instruction size
anyway, so, i'd just return 0 in the nacl case. And please, don't degrade this
in non-nacl case.

[eaeltsin, 2010-06-24 10:18:22]

On 2010/06/24 08:34:15, pasko wrote:
> LGTM
> 
> I really like the change! thanks! though I am not satisfied with my
> understanding of the quirks that this change may introduce.
> 
> Overall I would like to put as much comments as necessary right now, not
> postponing it to undefined dates. ChangeLog can be an exception from this
rule.
> 
> Also, TODOs really confuse me, I don't understand their scope. See below.
> 
> http://codereview.chromium.org/2873014/diff/1/3
> File gcc/gcc/config/i386/i386.c (left):
> 
> http://codereview.chromium.org/2873014/diff/1/3#oldcode8792
> gcc/gcc/config/i386/i386.c:8792: /* Extract the parts of an RTL expression
that
> is a valid memory address
> this text comments ix86_decompose_address, but for historical reasons it now
> prepends lea_match_address_operand, which is wrong. Please, fix this also.

Moved the comment down to x86_decompose_address.

> 
> http://codereview.chromium.org/2873014/diff/1/3
> File gcc/gcc/config/i386/i386.c (right):
> 
> http://codereview.chromium.org/2873014/diff/1/3#newcode8822
> gcc/gcc/config/i386/i386.c:8822: +	     TODO: what if (plus (plus a b) (plus c
> d)) ?  */
> another idea: s/TODO/REVIEW_QUESTION/

What do you mean by this?

I'd like to see a test that hits the case mentioned in the comment.
I actually don't understand why original version worked even without the fix
that is already done... Perhaps something is wrong somewhere else and such
expressions should never appear here?

> 
> http://codereview.chromium.org/2873014/diff/1/3#newcode11917
> gcc/gcc/config/i386/i386.c:11917: +print_operand_address_parts (FILE *file,
> const struct ix86_address *parts)
> thanks for this! a comment would be nice though

Done.

> 
> http://codereview.chromium.org/2873014/diff/1/3#newcode19595
> gcc/gcc/config/i386/i386.c:19595: +         TODO: factor out
> memory_address_parts_length and call it here with
> A general question: who is going to fix this TODO and when?  The comment
> suggests some easy refactoring operation, then it's a good candidate to be
done
> now. If the situation is opposite, i.e. it's hard to fix, then, as usual,
it'll
> never be fixed.
> 
> Please, identify your nickname as a TODO author or eliminate the TODO either
> way.

It seems gcc TODOs don't include the name of the responsible person, so I just
follow gcc's style :-)

I want to hear from Ian and Diego on this change before polishing it. If they
approve, I'll fix this and perhaps few more places. If not, this will be
rewritten anyway.

> 
> Also, I am pretty sure it's safe to return 0 because size computations are
used
> only for performance reasons. We are not precise about the instruction size
> anyway, so, i'd just return 0 in the nacl case. And please, don't degrade this
> in non-nacl case.

Nice catch! Fixed.