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