| Message ID | 1407345815-14551-13-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/06/14 11:19, David Malcolm wrote: > DF_REF_INSN looks up the "insn" field of the referenced df_insn_info. > This will eventually be an rtx_insn *, but for now is just an rtx. > > As further scaffolding: for now, convert DF_REF_INSN to a function, > adding a checked downcast to rtx_insn *. This can eventually be > converted back to macro when the field is an rtx_insn *. > > gcc/ > * df-core.c (DF_REF_INSN): New, using a checked cast for now. > * df.h (DF_REF_INSN): Convert from a macro to a function, so > that we can return an rtx_insn *. > > / > * rtx-classes-status.txt: Add DF_REF_INSN. OK. jeff
On Tue, 2014-08-12 at 15:20 -0600, Jeff Law wrote: > On 08/06/14 11:19, David Malcolm wrote: > > DF_REF_INSN looks up the "insn" field of the referenced df_insn_info. > > This will eventually be an rtx_insn *, but for now is just an rtx. > > > > As further scaffolding: for now, convert DF_REF_INSN to a function, > > adding a checked downcast to rtx_insn *. This can eventually be > > converted back to macro when the field is an rtx_insn *. > > > > gcc/ > > * df-core.c (DF_REF_INSN): New, using a checked cast for now. > > * df.h (DF_REF_INSN): Convert from a macro to a function, so > > that we can return an rtx_insn *. > > > > / > > * rtx-classes-status.txt: Add DF_REF_INSN. > OK. Thanks. Although this function gets converted back to a macro in patch 191, I just realized that in the meantime that it's not inlined, as is the case for some of the other macro->function conversions in patches 13-16. Do I need to convert them to inline functions with the appropriate headers, and is that regarded as a sufficiently trivial fix to the stuff you've already reviewed to not need re-review? (I will bootstrap&test). Or is it OK to suffer the performance hit as the patchkit lands, before they all become macros again in phase 4 of the patchkit? Note also that Jakub expressed concern about the effect of all these inline functions on the debugging experience, and there's this patch (awaiting review) which I believe addresses that: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00743.html Presumably similar changes to gdbinit.in should occur for the relevant headers (e.g. df.h in this case, though possibly targeted to just the new function - there are already quite a few inline functions in df.h) Dave
On 08/13/14 14:28, David Malcolm wrote: > Thanks. Although this function gets converted back to a macro in patch > 191, I just realized that in the meantime that it's not inlined, as is > the case for some of the other macro->function conversions in patches > 13-16. > > Do I need to convert them to inline functions with the appropriate > headers, and is that regarded as a sufficiently trivial fix to the stuff > you've already reviewed to not need re-review? (I will bootstrap&test). I'd just make it a follow-up. #237 ;-) > > Or is it OK to suffer the performance hit as the patchkit lands, before > they all become macros again in phase 4 of the patchkit? I think so. This is a transient state, and my goal is to have this stuff reviewed and get off the critical path before I go on PTO next week. > > Note also that Jakub expressed concern about the effect of all these > inline functions on the debugging experience, and there's this patch > (awaiting review) which I believe addresses that: > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00743.html Make it #238. > > Presumably similar changes to gdbinit.in should occur for the relevant > headers (e.g. df.h in this case, though possibly targeted to just the > new function - there are already quite a few inline functions in df.h) Yea, probably. jeff
On Wed, 2014-08-13 at 14:34 -0600, Jeff Law wrote: > On 08/13/14 14:28, David Malcolm wrote: > > Thanks. Although this function gets converted back to a macro in patch > > 191, I just realized that in the meantime that it's not inlined, as is > > the case for some of the other macro->function conversions in patches > > 13-16. > > > > Do I need to convert them to inline functions with the appropriate > > headers, and is that regarded as a sufficiently trivial fix to the stuff > > you've already reviewed to not need re-review? (I will bootstrap&test). > I'd just make it a follow-up. #237 ;-) Right, but these would be modifications to stuff that only exists between phases 1-3 of the kit, before going away in phase 4, so although it might be #237, it would need to be applied when the individual changes go in. > > Or is it OK to suffer the performance hit as the patchkit lands, before > > they all become macros again in phase 4 of the patchkit? > I think so. This is a transient state, and my goal is to have this > stuff reviewed and get off the critical path before I go on PTO next week. (FWIW I'm on PTO on Friday) Transient, yes, but given the amount of time for me to simply bootstrap each candidate patch, the non-inlined functions could last in trunk for a couple of weeks (there are only 168 hours in a week, and a bootstrap +regrtest takes about 3 hours on my box, so for 236 patches we're talking ~4 weeks of compute time just for that). I guess the underlying point here is that this is a big change and I'm trying to be fastidious here. Murphy's Law suggests I'm going to break at least *something* :( > > Note also that Jakub expressed concern about the effect of all these > > inline functions on the debugging experience, and there's this patch > > (awaiting review) which I believe addresses that: > > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00743.html > Make it #238. Right. I probably should move the link to the docs from the commit message to a comment in the gdbinit.in itself. Similar ordering considerations apply: I don't want to make debugging painful on trunk for a few weeks - so this kind of thing would need to go in as the inline functions go in. > > Presumably similar changes to gdbinit.in should occur for the relevant > > headers (e.g. df.h in this case, though possibly targeted to just the > > new function - there are already quite a few inline functions in df.h) > Yea, probably. I guess I'll try to get you patches for this by end of tomorrow. Thanks for all your reviewing Dave
On 08/13/14 18:11, David Malcolm wrote: > On Wed, 2014-08-13 at 14:34 -0600, Jeff Law wrote: >> On 08/13/14 14:28, David Malcolm wrote: >>> Thanks. Although this function gets converted back to a macro in patch >>> 191, I just realized that in the meantime that it's not inlined, as is >>> the case for some of the other macro->function conversions in patches >>> 13-16. >>> >>> Do I need to convert them to inline functions with the appropriate >>> headers, and is that regarded as a sufficiently trivial fix to the stuff >>> you've already reviewed to not need re-review? (I will bootstrap&test). >> I'd just make it a follow-up. #237 ;-) > > Right, but these would be modifications to stuff that only exists > between phases 1-3 of the kit, before going away in phase 4, so although > it might be #237, it would need to be applied when the individual > changes go in. If they're around just through phases #1-#3, then I wouldn't worry about it. > Transient, yes, but given the amount of time for me to simply bootstrap > each candidate patch, the non-inlined functions could last in trunk for > a couple of weeks (there are only 168 hours in a week, and a bootstrap > +regrtest takes about 3 hours on my box, so for 236 patches we're > talking ~4 weeks of compute time just for that). Well, I'd suggest a few things. 1. For the config/ changes, a full bootstrap is not necessary. For those targets which are embedded, just build a stage1 cross to the target to verify it builds and call it good. 2. For targets where you can bootstrap, go ahead and do so, but just on those targets. Some of these you can probably have going in parallel. 3. Some of the changes are so trivial that I'd squash them together in a single build/test cycle. I would even consider seeing all the scaffolding go in as a single chunk. It's nice to see the individuals during the review process, but I wouldn't lose any sleep if bundled those together. > > I guess the underlying point here is that this is a big change and I'm > trying to be fastidious here. Murphy's Law suggests I'm going to break > at least *something* :( Understood, but we don't want to be so fastidious that the time this stuff is in flux is so long that it creates more problems than it would if we took some sensible and safe shortcuts. Jeff
On Wed, 2014-08-13 at 20:55 -0600, Jeff Law wrote: > On 08/13/14 18:11, David Malcolm wrote: > > On Wed, 2014-08-13 at 14:34 -0600, Jeff Law wrote: > >> On 08/13/14 14:28, David Malcolm wrote: > >>> Thanks. Although this function gets converted back to a macro in patch > >>> 191, I just realized that in the meantime that it's not inlined, as is > >>> the case for some of the other macro->function conversions in patches > >>> 13-16. > >>> > >>> Do I need to convert them to inline functions with the appropriate > >>> headers, and is that regarded as a sufficiently trivial fix to the stuff > >>> you've already reviewed to not need re-review? (I will bootstrap&test). > >> I'd just make it a follow-up. #237 ;-) > > > > Right, but these would be modifications to stuff that only exists > > between phases 1-3 of the kit, before going away in phase 4, so although > > it might be #237, it would need to be applied when the individual > > changes go in. > If they're around just through phases #1-#3, then I wouldn't worry about > it. ...and indeed, having experimented with it, the inline function approach would need to include more header files: for example, an inline implementation of, say, BB_HEAD() in basic-block.h would look like this: inline rtx_insn *BB_HEAD (const_basic_block bb) { rtx insn = bb->il.x.head_; return safe_as_a <rtx_insn *> (insn); } but this requires everything including basic-block.h to know about safe_as_a <rtx_insn *> (rtx) and hence have access to is_a_helper <rtx_insn *> (rtx), and hence needs to include rtl.h i.e. either basic-block.h would need to include rtl.h, or everything including it would. Similar considerations apply to the macros in the various other header files. Touching the header file graph doesn't seem like a good thing to be doing for a transient effort during phases 1-3 of applying the kit, so I'll hold off on making that change, and go with the patches as-is. > > Transient, yes, but given the amount of time for me to simply bootstrap > > each candidate patch, the non-inlined functions could last in trunk for > > a couple of weeks (there are only 168 hours in a week, and a bootstrap > > +regrtest takes about 3 hours on my box, so for 236 patches we're > > talking ~4 weeks of compute time just for that). > Well, I'd suggest a few things. > > 1. For the config/ changes, a full bootstrap is not necessary. For > those targets which are embedded, just build a stage1 cross to the > target to verify it builds and call it good. > > 2. For targets where you can bootstrap, go ahead and do so, but just on > those targets. Some of these you can probably have going in parallel. > > 3. Some of the changes are so trivial that I'd squash them together in a > single build/test cycle. Thanks. FWIW I'm working on followup cleanups whilst waiting for the build/tests. > I would even consider seeing all the scaffolding go in as a single > chunk. It's nice to see the individuals during the review process, but > I wouldn't lose any sleep if bundled those together. > > > > > > I guess the underlying point here is that this is a big change and I'm > > trying to be fastidious here. Murphy's Law suggests I'm going to break > > at least *something* :( > Understood, but we don't want to be so fastidious that the time this > stuff is in flux is so long that it creates more problems than it would > if we took some sensible and safe shortcuts. (nods) Thanks again for the reviews Dave
diff --git a/gcc/df-core.c b/gcc/df-core.c index 9fdf6010..0dd8cc4 100644 --- a/gcc/df-core.c +++ b/gcc/df-core.c @@ -2532,3 +2532,9 @@ debug_df_chain (struct df_link *link) df_chain_dump (link, stderr); fputc ('\n', stderr); } + +rtx_insn *DF_REF_INSN (df_ref ref) +{ + rtx insn = ref->base.insn_info->insn; + return as_a_nullable <rtx_insn *> (insn); +} diff --git a/gcc/df.h b/gcc/df.h index 878f507..aabde63 100644 --- a/gcc/df.h +++ b/gcc/df.h @@ -647,7 +647,7 @@ struct df_d : BLOCK_FOR_INSN (DF_REF_INSN (REF))) #define DF_REF_BBNO(REF) (DF_REF_BB (REF)->index) #define DF_REF_INSN_INFO(REF) ((REF)->base.insn_info) -#define DF_REF_INSN(REF) ((REF)->base.insn_info->insn) +extern rtx_insn *DF_REF_INSN (df_ref ref); #define DF_REF_INSN_UID(REF) (INSN_UID (DF_REF_INSN(REF))) #define DF_REF_CLASS(REF) ((REF)->base.cl) #define DF_REF_TYPE(REF) ((REF)->base.type) diff --git a/rtx-classes-status.txt b/rtx-classes-status.txt index e57d775..68bbe54 100644 --- a/rtx-classes-status.txt +++ b/rtx-classes-status.txt @@ -10,5 +10,6 @@ Phase 6: use extra rtx_def subclasses: TODO TODO: "Scaffolding" to be removed ================================= +* DF_REF_INSN * SET_BB_HEAD, SET_BB_END, SET_BB_HEADER, SET_BB_FOOTER * SET_NEXT_INSN, SET_PREV_INSN