INSN_UIDs again (was Re: [PATCH 15/16] RTL frontend (rtl1), on top of dump reader)

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2016-10-06 at 15:30 +0200, Bernd Schmidt wrote:
> On 10/05/2016 06:15 PM, David Malcolm wrote:
> > +;; MEM[(struct isl_obj *)&obj1] = &isl_obj_map_vtable;
> > +(insn 1045 0 1046 2 (set (reg:SI 480)
> > +        (high:SI (symbol_ref:SI ("isl_obj_map_vtable")
> > +                    [flags 0xc0]
> > +                    <var_decl 0x7fa0363ea240
> > isl_obj_map_vtable>)))
> > +     y.c:12702 -1
> > +     (nil))
> > +(insn 1046 1045 1047 2 (set (reg/f:SI 479)
> > +        (lo_sum:SI (reg:SI 480)
> > +            (symbol_ref:SI ("isl_obj_map_vtable")
> > +               [flags 0xc0]
> > +               <var_decl 0x7fa0363ea240 isl_obj_map_vtable>)))
> > +     y.c:12702 -1
> > +     (expr_list:REG_EQUAL (symbol_ref:SI ("isl_obj_map_vtable")
> > +                             [flags 0xc0]
> > +                             <var_decl 0x7fa0363ea240
> > isl_obj_map_vtable>)
> > +        (nil)))
> 
> I hate saying this, since you've obviously spent a lot of effort, but
> I 
> think we're still at a too early stage to construct testcases. One
> issue 
> that came up while discussing previous patch kits is that the format 
> here is still too verbose, and I'd like to settle on a format first. 
> There's all manner of things that are pointless in a testcase and
> impede 
> readability:
> 
>   * INSN_UIDs and NEXT/PREV fields (could be constructed from
> scratch,    except for labels)

Currently (as of r241120) the "compact" dump format omits NEXT/PRIV
fields, and only dumps INSN_UIDs for CODE_LABELs (and within
LABEL_REFs).  I've reimplemented the loader so that it copes with this,
preserving INSN_UIDs where provided, and generating others, ensuring
uniqueness.

I found myself making this change to a test case
(gcc.dg/rtl/x86_64/into-cfglayout.c):

     /* The conversion to cfglayout should eliminate unconditional jump
        instructions...  */
-/* { dg-final { scan-rtl-dump "Removing jump 14." "into_cfglayout" } }  */
-/* { dg-final { scan-rtl-dump-not "jump_insn 14" "into_cfglayout" } }  */
-/* { dg-final { scan-rtl-dump-not "barrier 15" "into_cfglayout" } }  */
+/* { dg-final { scan-rtl-dump "Removing jump 13." "into_cfglayout" } }  */
+/* { dg-final { scan-rtl-dump-not "jump_insn 13" "into_cfglayout" } }  */
+/* { dg-final { scan-rtl-dump-not "barrier" "into_cfglayout" } }  */
 
 /* ...but conditional jumps should be preserved.  */
-/* { dg-final { scan-rtl-dump "jump_insn 10" "into_cfglayout" } }  */
+/* { dg-final { scan-rtl-dump "jump_insn 9" "into_cfglayout" } }  */

All of the INSN_UIDs being referred to are implicitly generated by the
loader.

Presumably we want to allow for test cases like this that verify that a
specific insn was affected by a pass e.g. scanning for
  "Removing jump
14",
but having the INSN_UIDs be autogenerated seems to make this very
fragile (and makes the scan-rtl-dump directives hard to read).

Hence I'm leaning towards having the dump format contain explicit INSN_UIDs for all insns, not just CODE_LABELs (but omitting the NEXT/PREV, those are redundant noise).

Currently the jump insn in question looks like this:

      (cjump_insn (set (pc)
                    (label_ref 20)) 
                 (nil))

With explicit INSN_UIDs it would look like this:

      (cjump_insn 13 (set (pc)
                    (label_ref 20)) 
                 (nil))

Thoughts?
Dave

>   * INSN_CODE_NUMBERs in both textual and number form.
>   * Various (nil) fields
>   * VAR_DECL addresses.

[...snip...]