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: 0005-Search-all-dominated-blocks-for-expressions-to-hoist.patch


On 07/19/10 12:39, Maxim Kuvyrkov wrote:



Just so I'm certain I understand the issue. Is the problem that the CALL_INSN is clobbering the value in the MEM, which normally wouldn't be a problem except that we can (in certain cases) place the MEM before the CALL_INSN. Right?

Correct.


One could argue this is a failing of
hoist_expr_reaches_here_p which fails to take the corner cases of
inserting at the end of a block into account.

While that's technically correct, I see hoist_expr_reaches_here_p as designed to perform it's job using antloc, transp, and other data sets, i.e., without examining the instruction stream. It seems simpler to handle corner cases like this one by cleaning up the data sets earlier in the pass.
But you don't know the target block for the insertion when you're pruning the data sets. Thus you don't know if the target block has any of the odd situations which require insertion earlier in the block. hoist_expr_reaches_here_p is supposed to determine if an expression, if inserted at the "end" of a specific block reaches a later point in the cfg unchanged. Since we know the target block of the insertion we can check for the various odd conditions that require insertion before the actual end of the target block.



Looking at insert_insn_end_basic_block we have another class of problems. Say the target block ends with a set & jump insn. The destination of the set might clobber a value needed by an insn hoisted to the end of the block. What a mess we've stumbled into.

In theory, "yes" it may happen that a set and/or jump insns modify memory or have other interesting side effects. In practice, we don't seem to have this problem with any of the ports.
The fact that we haven't stumbled across this bug doesn't mean the problem does not exist. I think we've largely been lucky because we don't run code hoisting unless optimizing for size and because the combiner is the pass most likely to create these insns and the combiner runs after gcse.

I'm not saying you have to address this problem right now (though a comment discussing the issue would be greatly appreciated).

+ if (!pre_p && MEM_P (e->expr))
+ /* Note memory references that can be clobbered by a call.
+ We do not split abnormal edges in HOIST, so would
+ a memory reference get hoisted along an abnormal edge,
+ it would be placed /before/ the call. Therefore, only
+ constant memory references can be hoisted along abnormal
+ edges. */
+ {
+ if (GET_CODE (XEXP (e->expr, 0)) == SYMBOL_REF
+ && CONSTANT_POOL_ADDRESS_P (XEXP (e->expr, 0)))
+ continue;

This is the only part I'm struggling with. It looks like you're trying to avoid setting prune_exprs if the MEM's address has certain properties (such as a reference to static memory). However, if my analysis of the problem is correct, that's not sufficient to solve the problem.

This code comes straight from compute_transpout.
I mis-understood what the code was checking for -- it's dealing with MEMs which are known to be constant and thus can't be clobbered by the call, which clearly isn't a problem. My bad.


I think at this point we should go forward with your patch and if you could add a pair of comments to expr_reaches_here_p and the pruning code which touch on the problems with set-and-jump insns as a separate patch, that would be greatly appreciated.


Thanks for your patience,
Jeff


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