On Thu, 16 Jul 2015, Andrew MacLeod wrote:
> On 07/16/2015 07:54 AM, Andrew MacLeod wrote:
> > On 07/16/2015 03:27 AM, Richard Biener wrote:
> > > On Wed, 15 Jul 2015, Andrew MacLeod wrote:
> > >
> > > > admittedly neither situation is very common I suspect, but it does seem
> > > > like a
> > > > hidden gotchya waiting to happen.
> > > I guess we either want to checking-assert that we never hit that
> > > special marker or handle it appropriately. Or even better avoid
> > > it in the first place (not sure why we have it - I suppose to allow
> > > modifying immediate uses of the current stmt from inside
> > > FOR_EACH_IMM_USE_STMT).
> > >
> > > For me single_imm_use_1 crashed on the NULL USE_STMT at
> > >
> > > if (!is_gimple_debug (USE_STMT (ptr)))
> > >
> > > so I presume all was fine until debug stmts were introduced
> > > (well, fine as in not crashing, not as in giving correct answers).
> > >
> > >
> > yes, It was probably still wrong, we just erred reporting that a real
> > single_use statemen't wasn't.
> >
> > The marker is unique in that the STMT field is NULL, which can't happen
> > otherwise.
> >
> > I'll think about how to efficiently get this right
> >
> > Andrew
> >
> We need to keep the marker because its we need it to update in a list that is
> being iterated upon.
>
> The affected routines are has_zero_uses, has_single_use, single_imm_use, and
> num_imm_uses. They can all be impacted by the presence of the marker.
>
> The fix the issue, we need to check USE_STMT for null before checking for
> is_gimple_debug() in all cases.
>
> It seemed to me that the shortcutting I was taking to check the simple cases
> first may not be saving us much... it looked like the executed code path ought
> to be pretty similar to a simple loop... similar number of compares and
> instructions and jumps. So I implemented has_zero_uses() and
> has_single_use() directly with a loop, and compared the generated code at -O.
>
> sure enough, the code path was pretty darn close. In fact, the routines
> ought to be generally faster.. without the shortcut tests the cases for 2 or
> more entries no longer require a function call, and they get a 2 entries
> "headstart" into the list since those first 2 iterations were basically
> duplicated by the shortcut code.
>
> num_imm_uses is slightly slower, but its only called from one or 2 places, and
> its not THAT much slower.
>
> Finally, given the extra work that single_imm_uses does, It seemed prudent to
> mostly leave it alone, just add the check.
>
> Want to give this a try and make sure it resolves the issue?
No time right now but knowing the symptom it indeed shoudl
> It bootstraps on x86_64-unknown-linux-gnu and shows no new regressions.
> OK for trunk?
Ok.
Thanks,
Richard.
>
> Andrew
>
> PS, assembly at -O2 for :
>
> // New version
> bool tryit1(tree t)
> {
> return has_single_use (t); // new implementation
> }
> <..>
> movq 48(%rdi), %rdx
> leaq 40(%rdi), %rsi
> xorl %eax, %eax
> cmpq %rdx, %rsi
> je .L4154
> .p2align 4,,10
> .p2align 3
> .L4152:
> movq 16(%rdx), %rcx
> testq %rcx, %rcx
> je .L4150
> cmpb $2, (%rcx)
> je .L4150
> testb %al, %al
> jne .L4155
> movl $1, %eax
> .L4150:
> movq 8(%rdx), %rdx
> cmpq %rdx, %rsi
> jne .L4152
> rep ret
> // Old version
> bool tryit2(tree t)
> {
> return has_single_use2 (t); // old implemenation
> }
>
> <...>
> movq %rdi, %rax
> movq 48(%rax), %rax
> leaq 40(%rdi), %rdi
> cmpq %rax, %rdi
> je .L4168
> cmpq 8(%rax), %rdi
> je .L4171
> xorl %edx, %edx
> xorl %esi, %esi
> jmp
> _Z16single_imm_use_1PK17ssa_use_operand_tPPS_PP21gimple_statement_base@PLT
> .p2align 4,,10
> .p2align 3
> .L4171:
> movq 16(%rax), %rax
> testq %rax, %rax
> je .L4168
> cmpb $2, (%rax)
> setne %al
> ret
> .p2align 4,,10
> .p2align 3
> .L4168:
> xorl %eax, %eax
> ret
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
