On 08/12/14 18:26, David Malcolm wrote:
Essentially zero in the work-to-date.
Of the 13 new subclasses of rtx, I only make major use of about half of
them; here are the frequencies (as reported by grep -w in my current
working copy):
class rtx_def; /* ~26000 for "rtx" */
class rtx_expr_list; /* 73 */
class rtx_insn_list; /* 130 */
class rtx_sequence; /* 71 */
class rtx_insn; /* ~4300 */
class rtx_real_insn; /* 17 */
class rtx_debug_insn; /* 9 */
class rtx_nonjump_insn; /* 5 */
class rtx_jump_insn; /* 9 */
class rtx_call_insn; /* 25 */
class rtx_jump_table_data; /* 41 */
class rtx_barrier; /* 25 */
class rtx_code_label; /* 176 */
class rtx_note; /* 63 */
i.e. rtx_real_insn is basically unused, as are rtx_debug_insn,
rtx_nonjump_insn and rtx_jump_insn.
I think there's a case for keeping the concrete subclasses (those last
three), but we can drop rtx_real_insn.
Let's go with that. I wouldn't be surprised if we find more cases where
we want the more concrete classes in the future, so I'm not worried
about their low usage at this point.
And we can always re-introduce the real-insn concept in the future if we
find the need. I guess I'm rather biased against deep inheritance
hierarchies after working on projects with unnecessarily deep
hierarchies in the past.
with an rtx_real_insn you're guaranteed at least a "uuBeiie". But
nothing is using that in the patches as they stand, so we can simply
drop the class.
And that's the one property I think argues to keep the intermediate
class. But with nothing using it right now, let's drop.
Perhaps the class hierarchy diagram in coretypes.h should gain the above
operand annotation?
Sure. Feel free to add that tidbit whenever you feel.
No strong opinion here. I think we added NULL_TREE/NULL_RTX. I could
possibly see extending that to the overall concept of "insn chain
things", but I think doing one for each subclass would probably be overkill.
In the absence of strong opinions, maybe we should proceed with the
patches as written i.e. *without* a NULL_INSN define.
OK. We can revisit if/when we make things in the chain separate from rtxs.
So out of curiosity, any thoughts on what other big things are out there
that need to be fixed. I'm keen to keep this stuff moving as much as
possible.
Arguably PATTERN() should require an rtx_insn * rather than a plain rtx,
but it would be an involved patch.
Yea. That's a good one. Probably a series unto itself at some point.
Other followups would be to reduce the number of as_a <rtx_insn *> in
the code; for example grepping for "uncast_" shows quite a few, a
pattern where I strengthen the type of a parameter as seen within the
function without needing to strengthen the param itself, where:
Yea. These are reasonable to attack independently.
A part of me really wonders if the 6 phases above should have been
submitted independently. ie, once the scaffolding was done why not go
ahead and get that reviewed & installed, then move on to phase2 patches.
I bring it up more for future work of a similar nature.
I realize that you had to do a fair amount of the later work to ensure
the scaffolding was right and so that we could see what the end result
would likely look like. But something feels like it could be staged better.
FWIW What you're seeing is the end result of a *lot* of
"git rebase -i", where I was splitting, combining, and reordering
patches: the phases didn't exist as fully-formed entities until I was
ready to send the patches.
Though I appreciate things were suboptimal here.
Understood. Just looking for a way where we an move this kind of work
forward without the level of pain on your side and without having to
wait for a 236 series patchset before it can be contributed. Maybe we
just have to accept that this kind of work is going to be difficult to
stage for reviews. Dunno, just feels like we ought to be able to make
it simpler for both of us and have smaller hunks staging in earlier.
Jeff
