Re: Fix handling of call clobbering readonly-result

Re: Fix handling of call clobbering readonly-result

[Richard Kenner]

     Thanks for the feedback. Your analysis about cse doing something wrong 
     appears confirmed in the RTL dumps for my reduced case with
     positive/negative functions (see below).

No, cse is fine.  And, as you suspected, this has nothing to do with your
change.

The bug is in gcse, which I know nothing about and which has essentially no
comments.

Why are we allowing code in GCC that has no comments in front of functions
that say what they do?  This is a really serious problem and one that I think
the SC needs to address.  Increasingly-large parts of GCC are completely
unmaintainable due to this lack of documentation.

No matter how useful a change can be, we need to take a very firm position
that it must not be installed if it is not properly documented.  The position
that "it's useful: we can always document it later" is clearly bogus: the
documentation never gets done.

In any event, what's going on is that we have in .addressof:

(insn 47 102 55 4 b3188 (set (reg/v:SI 49 [ x ])
        (const_int 100 [0x64])) 124 {*arm_movsi_insn} (nil)
    (nil))

(insn 55 47 56 4 b3188 (set (reg:SI 0 r0 [ x ])
        (reg/v:SI 49 [ x ])) 124 {*arm_movsi_insn} (nil)
    (expr_list:REG_EQUAL (const_int 100 [0x64])
        (insn_list:REG_LIBCALL 57 (nil))))

(call_insn/u 56 55 57 4 b3188 (parallel [
            (set (reg:SI 0 r0)
                (call (mem:SI (symbol_ref:SI ("^negative") <function_decl a5790 negative>) [0 S4 A32])
                    (const_int 0 [0x0])))
            (use (const_int 0 [0x0]))
            (clobber (reg:SI 14 lr))
        ]) 190 {*call_value_symbol} (nil)
    (expr_list:REG_EH_REGION (const_int -1 [0xffffffffffffffff])
        (nil))
    (expr_list (use (reg:SI 0 r0 [ x ]))
        (nil)))

(insn 57 56 60 4 b3188 (set (reg:SI 55)
        (reg:SI 0 r0)) 124 {*arm_movsi_insn} (nil)
    (insn_list:REG_RETVAL 55 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI ("^negative") <function_decl a5790 negative>)
                (expr_list (reg/v:SI 49 [ x ])
                    (nil)))
            (nil))))

gcse then modifies the SET_SRC of insn to be const_int 100.

The bug is that insn 55 is inside a libcall, so if you modify it, you must be
sure that a similar modification is made in the REG_EQUAL note at the end of
the libcall (see cse).

However, why is this optimization being made in the first place?  Changing
the source of an insn that sets a pseudo from a register to a constant is
*not* an "optimization" in this sort of case since it won't eliminate any
insns and can pessimize code in many cases.

In any event, why is gcse doing this "optimization"?  As I say, there is not
a single comment (or at least none in the proper place or one that I can find
via an obvious search string) saying what the pass is supposed to be doing,
but the name implies it's *local*.  Then why is it needed?  Doesn't it
already duplicate what cse is doing?  cse knows far more about how to make
this sort of decisions than the existing code (at least as far as I can see)
and, in this case, gets it right when gcse does not.

My feeling is that the fix for this bug is to remove the local_cprop_*
functions from gcse.  They are totally undocumented, appear either
unnecessary or to pessimize code, and they have at least this bug.

Does anybody plan on adding the proper documentation for this "local_cprop"
pass (including specifically how what it does differs from what cse does and
why it's useful)?

Does anybody know the code well enough to fix this bug?

Re: Fix handling of call clobbering readonly-result

[Joe Buck]

On Fri, May 09, 2003 at 07:58:18AM -0400, Richard Kenner wrote:
> Why are we allowing code in GCC that has no comments in front of functions
> that say what they do?  This is a really serious problem and one that I think
> the SC needs to address.  Increasingly-large parts of GCC are completely
> unmaintainable due to this lack of documentation.

We can do things: assure that it doesn't get worse, and try to get
documentation for the missing pieces.

First, let's not let it get worse: no more CVS commits that add an
undocumented function.  This is not a hard standard to enforce.

Second, we have CVS history.  It shouldn't be hard to track down those
who created undocumented functions recently and ask those folks to
contribute some documentation.  The longer we wait, the harder it will be.

Third, Richard, I really wish you had a mailer that created proper
References or In-Reply-To headers.  Big threads that you participate in are
extremely difficult to follow, as it's not possible to determine who you
are replying to.