This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix handling of call clobbering readonly-result


     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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]