Re: [PATCH, RFC]: Next stage1, refactoring: propagating rtx subclasses

[Jeff Law <law at redhat dot com>]

On 03/30/2015 10:37 PM, Mikhail Maltsev wrote:
> Hi!
>
> I'm currently working on the proposed task of replacing rtx objects
> (i.e. struct rtx_def) with derived classes. I would like to get some
> feedback on this work (it's far from being finished, but basically I
> would like to know, whether my modifications are appropriate, e.g. one
> may consider that this is "too much" for just refactoring, because
> sometimes they involve small modification of semantics).
>
> The attached patch is not well tested, i.e. I bootstrapped and regtested
> it only on x86_64, but I'll perform more extensive testing before
> submitting the next version.
>
> The key points I would like to ask about:
>
> 1. The original task was to replace the rtx type by rtx_insn *, where it
> is appropriate. But rtx_insn has several derived classes, such as
> rtx_code_label, for example. So I tried to use the most derived type
> when possible. Is it OK?
Definitely.  In general the idea here is to exploit the static type 
checking done in the compiler to avoid runtime checking and failures.


> 2. Not all of these "type promotions" can be done by just looking at
> function callers and callees (and some functions will only be generated
> during the build of some rare architecture) and checks already done in
> them. In a couple of cases I referred to comments and my general
> understanding of code semantics. In one case this actually caused a
> regression (in the patch it is fixed, of course), because of somewhat
> misleading comment (see "live_label_rtx" function added in patch for
> details) The question is - are such changes OK for refactoring (or it
> should strictly preserve semantics)?
They're OK, but it may be easier to run things through the review 
process if refactoring is kept separate from strengthening the type 
checking.


> 3. In lra-constraints.c I added a new class rtx_usage_list, which, IMHO,
> allows to group the functions which work with usage list in a more
> explicit manner and make some conditions more self-explaining. I hope
> that Vladimir Makarov (in this case, because it concerns LRA) and other
> authors will not object against such "intrusion" into their code (or
> would rather tell me what should be fixed in my patch(es), instead of
> just refusing to apply it/them). In general, are such changes OK or
> should better be avoided?
>
> A couple of questions related to further work:
I don't see anything inherently wrong with this concept.  Though again, 
I'd suggest separating out these changes from type safety work.

> 1. I noticed that emit_insn function, in fact, does two kinds of things:
> it can either add it's argument to the chain, or, if the argument is a
> pattern, it creates a new instruction based on that pattern. Shouldn't
> this logic be separated in the callers?
That would be wise.  There's probably several of these kinds of things 
lurking around.

> 2. Are there any plans on implementing a better class hierarchy on AST's
> ("union tree_node" type). I see that C++ FE uses a huge number of macros
> (which check TREE_CODE and some boolean flags). Could this be improved
> somehow?
It's in progress and I'm hoping Andrew will be in a position to post 
this work soon.

jeff