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 7/19/10 10:08 PM, Jeff Law wrote:
On 07/17/10 10:41, Maxim Kuvyrkov wrote:
On 7/16/10 10:37 PM, Jeff Law wrote:
On 07/15/10 13:22, Maxim Kuvyrkov wrote:
On 7/15/10 8:06 PM, Jeff Law wrote:
...
? See the e->flags & EDGE_ABNORMAL test prior to removing elements of
trapping_expr from antloc or transp.

I missed that point that we should avoid emitting anything at the ends of basic blocks with abnormal edges.

Is the attached patch OK?
Yes as long as it passes the usual bootstrap & regression test.

Well, after the regtest I now know the exact purpose of transpout. It is to avoid moving memory references across calls that can clobber memory. Without the checks done in compute_transpout a memory reference can be moved along an abnormal edge and, when that happens, it gets placed /before/ the call.
Placement before the call is intentional.

Certainly.


The CALL_INSN (if it has
abnormal edges) has to be considered to change control flow. If the
hoisted insn is placed after the CALL_INSN, the hoisted insn may not be
executed if the CALL_INSN throws (for example).
insert_insn_end_basic_block contains the logic to determine where in a
block to place the new insn.

I *think* PRE avoids this by only inserting in a block where it knows
the inserted insn is safe to place anywhere within the block. In the
case of a MEM, we must have already determined that the block (and thus
any CALL_INSN within the block) doesn't clobber the MEM. Hoisting is a
little different in that it only verifies that it should be safe to
place the MEM at the end of block.

Agreed.



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.



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.


+ 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.


Why do you think these checks aren't sufficient? A memory reference to a constant pool is, well, a constant.

It may be that the second check

	      if (MEM_READONLY_P (e->expr)
		  && !MEM_VOLATILE_P (e->expr)
		  && MEM_NOTRAP_P (e->expr))
		/* Constant memory reference, e.g., a PIC address.  */
		continue;

implies CONSTANT_POOL_ADDRESS_P, but I prefer to err on the side of caution have both checks in place.

Thanks,

--
Maxim Kuvyrkov
CodeSourcery
maxim@codesourcery.com
(650) 331-3385 x724


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