Chromium Code Reviews (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

Issue 2873014: [gcc] Use new constraint for LEA address operands. (Closed)

10 years, 6 months ago by eaeltsin
9 years, 7 months ago
Base URL:


[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.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fixes for code review. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -74 lines) Patch
M gcc/gcc/config/i386/ View 1 chunk +4 lines, -0 lines 0 comments Download
M gcc/gcc/config/i386/i386.c View 22 chunks +275 lines, -54 lines 0 comments Download
M gcc/gcc/config/i386/ View 1 chunk +8 lines, -8 lines 0 comments Download
M gcc/gcc/config/i386/ View 1 chunk +1 line, -11 lines 0 comments Download
M gcc/gcc/gensupport.c View 1 chunk +0 lines, -1 line 0 comments Download


Total messages: 3 (0 generated)
10 years, 6 months ago (2010-06-23 15:40:46 UTC) #1
pasko-google - do not use
LGTM I really like the change! thanks! though I am not satisfied with my understanding ...
10 years, 6 months ago (2010-06-24 08:34:15 UTC) #2
10 years, 6 months ago (2010-06-24 10:18:22 UTC) #3
On 2010/06/24 08:34:15, pasko wrote:
> 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
> Also, TODOs really confuse me, I don't understand their scope. See below.
> File gcc/gcc/config/i386/i386.c (left):
> gcc/gcc/config/i386/i386.c:8792: /* Extract the parts of an RTL expression
> 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.

> File gcc/gcc/config/i386/i386.c (right):
> 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?

> 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


> 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
> now. If the situation is opposite, i.e. it's hard to fix, then, as usual,
> 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
> 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.

Powered by Google App Engine
This is Rietveld 408576698