Re: [PATCH] Reduce conservativeness in REE using machine model (issue6631066)

[Teresa Johnson <tejohnson at google dot com>]

On Fri, Oct 12, 2012 at 1:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 11, 2012 at 02:44:12PM -0700, Teresa Johnson wrote:
>> Revised patch to address conservative behavior in redundant extend
>> elimination that was resulting in redundant extends not being
>> removed. Now uses a new target hook machine_mode_from_attr_mode
>> which is currently enabled only for i386.
>
> I still don't like it, the hook still is about how it is implemented
> instead of what target property it wants to ask (the important thing
> there is that a {QI,HI} -> SImode zero extension instruction on x86_64
> performs {QI,HI} -> DImode extension actually).  That isn't the case for any
> other modes, isn't the case for sign extension etc.

That's true, although for sign extends the attr modes being set in
i386.md ensure that this won't do the wrong thing as the the attr
modes in the machine desc file match the machine_mode. However, this
ends up leading to the conservative behavior remaining for sign
extends (see testcase below).

>
> Can you please post a testcase first?

This was exposed by the snappy decompression code. However, I was able
to reproduce it by modifying the testcase submitted with the fix that
introduced this checking (gcc.c-torture/execute/20111227-1.c for
http://gcc.gnu.org/ml/gcc-patches/2011-12/msg01744.html). This test
case was failing because a sign_extend was being combined with a
zero_extend, so the instruction code check below fixed it, and the
mode check was unnecessary:

      /* Second, make sure the reaching definitions don't feed another and
         different extension.  FIXME: this obviously can be improved.  */
      for (def = defs; def; def = def->next)
        if ((idx = def_map[INSN_UID(DF_REF_INSN (def->ref))])
            && (cand = &VEC_index (ext_cand, *insn_list, idx - 1))
            && (cand->code != code || cand->mode != mode))

Here is my modified test case that exposes the conservative behavior I
saw in snappy:

------------------------
extern void abort (void);

unsigned short s;
unsigned int i;
unsigned long l;
unsigned char v = -1;

void __attribute__((noinline,noclone))
bar (int t)
{
  if (t == 2 && s != 0xff)
    abort ();
  if (t == 1 && i != 0xff)
    abort ();
  if (t == 0 && l != 0xff)
    abort ();
}

void __attribute__((noinline,noclone))
foo (unsigned char *a, int t)
{
  unsigned char r = v;

  if (t == 2)
    s = (unsigned short) r;
  else if (t == 1)
    i = (unsigned int) r;
  else if (t == 0)
    l = (unsigned long) r;
  bar (t);
}

int main(void)
{
  foo (&v, 0);
  foo (&v, 1);
  foo (&v, 2);
  return 0;
}
-----------------------

With trunk, there are currently 3 movzbl generated for foo():

	movzbl	v(%rip), %eax
	movzbl	%al, %eax
	movzbl	%al, %eax

With my fix this goes down to 1 movzbl. However, if the test case is
modified to use signed instead of unsigned, we still end up with 3
movsbl, of which 2 are redundant:

 	movsbw	v(%rip), %ax
	movsbq	%al, %rax
	movsbl	%al, %eax

A single movsbq will suffice. But because of the attr mode settings
for sign extends I mentioned above, my patch does not help here.

>
> Given the recent ree.c changes to remember the performed operations and
> their original modes (struct ext_modified), perhaps the
> "Second, make sure the reaching definitions don't feed another and"...
> check could be made less strict or even removed, but for that a testcase is
> really needed.

I believe that we can remove the mode check from the above code
altogether. The reason is that the ree.c code will always select the
widest mode when combining extends (in merge_def_and_ext). So with a
simple change to the ree.c code to simply avoid the mode checking both
the unsigned and signed cases get addressed. In the signed case we are
left with a single movs:

	movsbq	v(%rip), %rax

I didn't see any test failures in a x86_64-unknown-linux-gnu bootstrap
and regression test run. If this seems reasonable, I can follow up
with the patch (which is trivial), and I can also submit the 2 new
test cases (the signed test case is included below).

Thanks,
Teresa


extern void abort (void);

signed short s;
signed int i;
signed long l;
signed char v = -1;

void __attribute__((noinline,noclone))
bar (int t)
{
  if (t == 2 && s != -1)
    abort ();
  if (t == 1 && i != -1)
    abort ();
  if (t == 0 && l != -1)
    abort ();
}

void __attribute__((noinline,noclone))
foo (signed char *a, int t)
{
  signed char r = v;

  if (t == 2)
    s = (signed short) r;
  else if (t == 1)
    i = (signed int) r;
  else if (t == 0)
    l = (signed long) r;
  bar (t);
}

int main(void)
{
  foo (&v, 0);
  foo (&v, 1);
  foo (&v, 2);
  return 0;
}



>
>         Jakub



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413