On Thu, 2016-10-06 at 15:53 -0400, David Malcolm wrote:
> 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.
>
> We have to construct testcases in order to see what the testcase
> format
> should look like - it's hard to talk about things without examples.
>
> > 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)
>
> A benefit of keeping the INSN_UIDs is that if you've spent half an
> hour
> single-stepping through RTL modification and know that INSN_UID 1045
> is
> the one that gets corrupted, you can have that insn have UID 1045 in
> the testcase. Otherwise the UIDs get determined by the parser, and
> can
> change if other insns get inserted.
>
> Explicit UIDs let you refer to specific insns when discussing a test
> case.
>
> > * INSN_CODE_NUMBERs in both textual and number form.
> > * Various (nil) fields
>
> FWIW these (nil) fields signify the end of various expr_list chains.
> If we emit them, there are places in the format where it would become
> ambiguous as to which field is getting an expr_list.
>
> > * VAR_DECL addresses.
>
> Here's a revised version of that example
> (gcc/testsuite/rtl.dg/aarch64/pr71779.rtl), with the things you
> mention
> removed, and with some indentation to aid readability:
>
> ;; { dg-do compile { target aarch64-*-* } }
> ;; { dg-options "-fsingle-pass=rtl-cse1 -fdump-rtl-cse1" }
>
> (function "fragment"
> (insn-chain
> ;; MEM[(struct isl_obj *)&obj1] = &isl_obj_map_vtable;
> (insn 2 (set (reg:SI 480)
> (high:SI (symbol_ref:SI ("isl_obj_map_vtable")
> [flags 0xc0] <var_decl
> isl_obj_map_vtable>)))
> y.c:12702)
> (insn 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
> (expr_list:REG_EQUAL (symbol_ref:SI ("isl_obj_map_vtable")
> [flags 0xc0]
> <var_decl isl_obj_map_vtable>)))
> (insn 2 (set (reg:DI 481)
> (subreg:DI (reg/f:SI 479) 0)) y.c:12702)
> (insn 2 (set (zero_extract:DI (reg/v:DI 191 [ obj1D.17368 ])
> (const_int 32 [0x20])
> (const_int 0 [0]))
> (reg:DI 481)) y.c:12702)
> ;; Extra insn, to avoid all of the above from being deleted by
> DCE
> (insn 2 (set (mem:DI (reg:DI 191) [1 i+0 S4 A32])
> (const_int 1 [0x1])))
>
> ) ;; insn-chain
> ) ;; function
>
> This is with the optional cfg and crtl directives omitted.
>
> Does the above look acceptable?
>
> The "2" values after each "insn" are the basic block indices. Should
> these be omitted also?
If we do that, one approach could be to introduce a (basic-block)
directive into the (insn-chain), giving something like this:
;; { dg-do compile { target aarch64-*-* } }
;; { dg-options "-fsingle-pass=rtl-cse1 -fdump-rtl-cse1" }
(function "fragment"
(insn-chain
(basic-block 2
;; MEM[(struct isl_obj *)&obj1] = &isl_obj_map_vtable;
(insn (set (reg:SI 480)
(high:SI (symbol_ref:SI ("isl_obj_map_vtable")
[flags 0xc0] <var_decl isl_obj_map_vtable>)))
y.c:12702)
(insn (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
(expr_list:REG_EQUAL (symbol_ref:SI ("isl_obj_map_vtable")
[flags 0xc0]
<var_decl isl_obj_map_vtable>)))
(insn (set (reg:DI 481)
(subreg:DI (reg/f:SI 479) 0)) y.c:12702)
(insn (set (zero_extract:DI (reg/v:DI 191 [ obj1D.17368 ])
(const_int 32 [0x20])
(const_int 0 [0]))
(reg:DI 481)) y.c:12702)
;; Extra insn, to avoid all of the above from being deleted by DCE
(insn (set (mem:DI (reg:DI 191) [1 i+0 S4 A32])
(const_int 1 [0x1])))
) ;; basic-block 2
) ;; insn-chain
) ;; function
> My hope here is to that the input format is something that existing
> gcc
> developers will be comfortable reading, which is why I opted for
> parsing the existing output - and I'm nervous about moving away too
> much from the existing format. In particular, I want the format to
> be
> something that can be automatically printed and roundtripped, and for
> it to be useful to look at when in the debugger.
>
> Some other possible changes: locations could do with a wrapper,
> and/or
> to quote the filename, since otherwise we can't cope with spaces in
> filenames). So instead of:
>
> (insn 2 (set (reg:DI 481)
> (subreg:DI (reg/f:SI 479)
> 0)) y.c:12702)
>
> we could have:
>
> (insn 2 (set (reg:DI 481)
> (subreg:DI (reg/f:SI 479) 0)) "y.c":12702)
>
> or:
>
> (insn 2 (set (reg:DI 481)
> (subreg:DI (reg/f:SI 479)
> 0)) (srcloc "y.c" 12702))
>
> Any preferences on these?
>
> > What does the RTL reader actually do with MEM_ATTRs?
>
> It parses them, where it can parse the MEM_EXPR, at least (it can
> only
> handle simple decl names for MEM_EXPR).
>
> > Do these survive
> > the dump/read process?
>
> Yes, for the MEM_EXPR cases above. So it will successfully parse the
> "[1 i+0 S4 A32]" above.
>
> > There was also a testcase where virtual regs still occurred with
> > register number, I think it would be best to disallow this and add
> > the
> > ability to parse "(reg:DI virtual-outgoing-args)".
>
> When we discussing pseudos you expressed a need to keep printing the
> regno for REGs, so that the dump format is usable in the debugger,
> for
> array indices etc. Hence I'd prefer to keep printing the regno for
> all
> regnos, including virtuals.
>
> > Also I think I'd prefer testcases submitted separately from the RTL
> > frontend. It seems we have two different frontend approaches here,
> > one
> > RTL frontend and one modification for the C frontend - which do you
> > prefer?
>
> Is it acceptable to do both? I'd like to have an actual RTL
> frontend,
> though this punts on handling structs etc, whereas Richi seems to
> prefer the modification to the C frontend, since it better supports
> structs, aliasing, etc. At this point, whichever gets me past patch
> review...
>
> Dave
