| Message ID | 87tx89abk6.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On 05/28/2014 04:32 PM, Richard Sandiford wrote: > While working on patches to speed up the handling of constraints, > I hit some behaviour in ira_get_dup_out_num that looked unintentional: > > - the check for output operands was part of the !ignored_p condition > so would be skipped if the first alternative is disabled/excluded. > > - the first disabled/excluded alternative stops all following alternatives > from being processed, since we get "stuck" in the first part of the > "if" statement and never increment curr_alt. > > This seems to have some effect on the testsuite. E.g. at -O2 > gcc.c-torture/compile/20071117-1.c has changes like: > > .LCFI2: > movq %rsp, %rbx > subq %rax, %rsp > - leaq 15(%rsp), %rax > - andq $-16, %rax > - movq %rax, %rdi > + leaq 15(%rsp), %rdi > + andq $-16, %rdi > call bar > xorl %esi, %esi > movq %rbx, %rsp > > There are also some cases where the change introduces a move though. > E.g. gcc.c-torture/compat/struct-ic.c has: > > movabsq $4294967296, %rdx > addq $8, %rsp > .LCFI4: > - andq %rdi, %rax > + andq %rax, %rdi > + movq %rdi, %rax > orq %rdx, %rax > ret > .L9: > > But AFAICT the patch is what was originally intended. > > Tested on x86_64-linux-gnu. OK to install? > > Ok, Richard. Thanks for fixing this.
Index: gcc/ira.c =================================================================== --- gcc/ira.c 2014-05-28 20:48:25.747321188 +0100 +++ gcc/ira.c 2014-05-28 21:31:22.769217059 +0100 @@ -1979,33 +1979,33 @@ ira_get_dup_out_num (int op_num, HARD_RE if (op_num < 0 || recog_data.n_alternatives == 0) return -1; - use_commut_op_p = false; + /* We should find duplications only for input operands. */ + if (recog_data.operand_type[op_num] != OP_IN) + return -1; str = recog_data.constraints[op_num]; + use_commut_op_p = false; for (;;) { #ifdef EXTRA_CONSTRAINT_STR op = recog_data.operand[op_num]; #endif - for (ignore_p = false, original = -1, curr_alt = 0;;) + for (curr_alt = 0, ignore_p = !TEST_HARD_REG_BIT (alts, curr_alt), + original = -1;;) { c = *str; if (c == '\0') break; - if (c == '#' || !TEST_HARD_REG_BIT (alts, curr_alt)) + if (c == '#') ignore_p = true; else if (c == ',') { curr_alt++; - ignore_p = false; + ignore_p = !TEST_HARD_REG_BIT (alts, curr_alt); } else if (! ignore_p) switch (c) { - /* We should find duplications only for input operands. */ - case '=': - case '+': - goto fail; case 'X': case 'p': case 'g':
While working on patches to speed up the handling of constraints, I hit some behaviour in ira_get_dup_out_num that looked unintentional: - the check for output operands was part of the !ignored_p condition so would be skipped if the first alternative is disabled/excluded. - the first disabled/excluded alternative stops all following alternatives from being processed, since we get "stuck" in the first part of the "if" statement and never increment curr_alt. This seems to have some effect on the testsuite. E.g. at -O2 gcc.c-torture/compile/20071117-1.c has changes like: .LCFI2: movq %rsp, %rbx subq %rax, %rsp - leaq 15(%rsp), %rax - andq $-16, %rax - movq %rax, %rdi + leaq 15(%rsp), %rdi + andq $-16, %rdi call bar xorl %esi, %esi movq %rbx, %rsp There are also some cases where the change introduces a move though. E.g. gcc.c-torture/compat/struct-ic.c has: movabsq $4294967296, %rdx addq $8, %rsp .LCFI4: - andq %rdi, %rax + andq %rax, %rdi + movq %rdi, %rax orq %rdx, %rax ret .L9: But AFAICT the patch is what was originally intended. Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * ira.c (ira_get_dup_out_num): Check for output operands at the start of the loop. Handle cases where an included alternative follows an excluded one.