The patch forces any insn that has a barrier after it be
control_flow_insn_p thus making it impossible for barrier to appear
inside basic block. An example of such insn is
(insn (trap_if (const_int 1))) on i586-pc-linux-gnu.

This sounds like a kludge to me, and the comment in the patch has nothing to
do with the problem AFAICS. I think that we should always be able to tell
whether an insn is control_flow_insn_p by looking at its pattern only (modulo
the presence of EH stuff).

The patch forces any insn that has a barrier after it be
control_flow_insn_p thus making it impossible for barrier to appear
inside basic block. An example of such insn is
(insn (trap_if (const_int 1))) on i586-pc-linux-gnu.

I'll commit the following once testing is complete.

* cfgbuild.c (control_flow_insn_p): Return TRUE for unconditional
trap instructions.

Eric Botcazou wrote:

Index: cfgbuild.c

cfgbuild.c(revision 123843)
cfgbuild.c(working copy)
@@ -120,6 +120,11 @@ control_flow_insn_p (rtx insn)
|| can_throw_internal (insn));

case INSN:
+ /* Treat trap instructions like noreturn calls (same provision). */
+ if (GET_CDE (PATTERN (insn)) == TRAP_IF
+ && XEXP (PATTERN (insn), 0) == const1_rtx)
+return true;
+
return (flag_non_call_exceptions && can_throw_internal (insn));

case BARRIER:

Did you consider fixing cfgrtl.c: rtl_verify_flow_info_1 () as well?
The workaround for a barrier inside a basic block should no longer be
needed with you patch.

Did you consider fixing cfgrtl.c: rtl_verify_flow_info_1 () as well?
The workaround for a barrier inside a basic block should no longer be
needed with you patch.
This would be a separate patch, not appropriate for the 4.2 branch.

* cfgbuild.c (control_flow_insn_p): Return TRUE for unconditional
trap instructions.

void f7(int p)
{
if (p)
__builtin_trap();
else
__builtin_trap();
}

doesn't pass at because of the following assertion in the scheduler:

/* Move INSN. Reemit notes if needed. Update CFG, if needed. */
static void
move_insn (rtx insn)
{
rtx last = last_scheduled_insn;

if (PREV_INSN (insn) != last)
{
basic_block bb;
rtx note;
int jump_p = 0;

bb = BLCK_FR_INSN (insn);

/* BB_HEAD is either LABEL or NTE. */
gcc_assert (BB_HEAD (bb) != insn);

if (BB_END (bb) == insn)
/* If this is last instruction in BB, move end marker one
instruction up. */
{
/* Jumps are always placed at the end of basic block. */
jump_p = control_flow_insn_p (insn);

gcc_assert (!jump_p
|| ((current_sched_info->flags & SCHED_RGN)
&& IS_SPECULATIN_BRANCHY_CHECK_P (insn))
|| (current_sched_info->flags & SCHED_EBB));

IS_SPECULATIN_BRANCHY_CHECK_P is false for the trap insn.

The check looks questionable to me, but we probably should not be trying to
schedule an unconditional trap in the first place, hence the revised patch.

Bootstrapped/regtested on x86_64-suse-linux, I'll install it on both branches
if scheduler specialists don't oppose.

2007-04-17 Eric Botcazou <ebotcazou (AT) libertysurf (DOT) fr>

PR rtl-optimization/29841
* cfgbuild.c (control_flow_insn_p): Return TRUE for unconditional
trap instructions.
* sched-deps.c (sched_analyze_insn): Prevent all non-jump instructions
that may cause control flow transfer from being moved.

2007-04-17 Eric Botcazou <ebotcazou (AT) libertysurf (DOT) fr>

* gcc.dg/invalid-call-1.c: New test.

* sched-deps.c (sched_analyze_insn): Prevent all non-jump instructions
that may cause control flow transfer from being moved.

Will this not make it impossible to use speculation on trapping memory
accesses on ia64?

The only new case for which it triggers is unconditional trap instructions.

Eric Botcazou wrote:
>* cfgbuild.c (control_flow_insn_p): Return TRUE for unconditional
>trap instructions.

void f7(int p)
{
if (p)
__builtin_trap();
else
__builtin_trap();
}

doesn't pass at because of the following assertion in the scheduler:

/* Move INSN. Reemit notes if needed. Update CFG, if needed. */
static void
move_insn (rtx insn)
{
rtx last = last_scheduled_insn;

if (PREV_INSN (insn) != last)
{
basic_block bb;
rtx note;
int jump_p = 0;

bb = BLCK_FR_INSN (insn);

/* BB_HEAD is either LABEL or NTE. */
gcc_assert (BB_HEAD (bb) != insn);

if (BB_END (bb) == insn)
/* If this is last instruction in BB, move end marker one
instruction up. */
{
/* Jumps are always placed at the end of basic block. */
jump_p = control_flow_insn_p (insn);

gcc_assert (!jump_p
|| ((current_sched_info->flags & SCHED_RGN)
&& IS_SPECULATIN_BRANCHY_CHECK_P (insn))
|| (current_sched_info->flags & SCHED_EBB));

IS_SPECULATIN_BRANCHY_CHECK_P is false for the trap insn.

The check looks questionable to me, but we probably should not be trying to
schedule an unconditional trap in the first place, hence the revised patch.

This assert checks that we are not trying to move a jump that we are not
supposed to move. Scheduler has two front ends: sched-rgn and
sched-ebb. Sched-ebb allows jumps to be moved while sched-rgn in all
but one cases prohibits it (via sched-rgn.c: add_branch_dependences ()).
The one case sched-rgn makes an exception is speculation checks. And
that it what the assert checks. I hope sometime in the future to finish
support for jump movement in sched-rgn.

Index: sched-deps.c

sched-deps.c(revision 123843)
sched-deps.c(working copy)
@@ -1585,8 +1585,10 @@ sched_analyze_insn (struct deps *deps, r

/* If this instruction can throw an exception, then moving it changes
where block boundaries fall. This is mighty confusing elsewhere.
- Therefore, prevent such an instruction from being moved. */
- if (can_throw_internal (insn))
+ Therefore, prevent such an instruction from being moved. Same for
+ non-jump instructions that define block boundaries. */
+ if (((CALL_P (insn) || JUMP_P (insn)) && can_throw_internal (insn))
+ || (NNJUMP_INSN_P (insn) && control_flow_insn_p (insn)))
reg_pending_barrier = MVE_BARRIER;

/* Add dependencies if a scheduling barrier was found. */

This is better to fix in add_branch_dependences ().

And you tested this on ia64 with -fsched-spec-load and
-fsched-spec-load-dangerous?
No, I just read the code.

This assert checks that we are not trying to move a jump that we are not
supposed to move. Scheduler has two front ends: sched-rgn and
sched-ebb. Sched-ebb allows jumps to be moved while sched-rgn in all
but one cases prohibits it (via sched-rgn.c: add_branch_dependences ()).
The one case sched-rgn makes an exception is speculation checks. And
that it what the assert checks.

Sorry for the vague comment. What I find questionable is

/* Jumps are always placed at the end of basic block. */
jump_p = control_flow_insn_p (insn);

control_flow_insn_p doesn't return TRUE only for jumps, it can return TRUE for
NNJUMP_INSN_P insn (e.g. trapping loads with -fnon-call-exceptions). The
assert

gcc_assert (!jump_p
|| ((current_sched_info->flags & SCHED_RGN)
&& IS_SPECULATIN_BRANCHY_CHECK_P (insn))
|| (current_sched_info->flags & SCHED_EBB));

seems to expect a true jump. Anyway, this doesn't matter anymore in our case.

@@ -1585,8 +1585,10 @@ sched_analyze_insn (struct deps *deps, r

/* If this instruction can throw an exception, then moving it changes
where block boundaries fall. This is mighty confusing elsewhere.
- Therefore, prevent such an instruction from being moved. */
- if (can_throw_internal (insn))
+ Therefore, prevent such an instruction from being moved. Same for
+ non-jump instructions that define block boundaries. */
+ if (((CALL_P (insn) || JUMP_P (insn)) && can_throw_internal (insn))
+ || (NNJUMP_INSN_P (insn) && control_flow_insn_p (insn)))
reg_pending_barrier = MVE_BARRIER;

/* Add dependencies if a scheduling barrier was found. */

This is better to fix in add_branch_dependences ().

Why would we schedule these instructions in EBB mode and not in RGN mode?

Eric Botcazou wrote:
>This assert checks that we are not trying to move a jump that we are not
>supposed to move. Scheduler has two front ends: sched-rgn and
>sched-ebb. Sched-ebb allows jumps to be moved while sched-rgn in all
>but one cases prohibits it (via sched-rgn.c: add_branch_dependences ()).
>The one case sched-rgn makes an exception is speculation checks. And
>that it what the assert checks.

Sorry for the vague comment. What I find questionable is

/* Jumps are always placed at the end of basic block. */

The comment should be changed to something like /* Instructions that
affect control flow always are placed at the end of basic block. */

jump_p = control_flow_insn_p (insn);

control_flow_insn_p doesn't return TRUE only for jumps, it can return TRUE for
NNJUMP_INSN_P insn (e.g. trapping loads with -fnon-call-exceptions). The
assert

gcc_assert (!jump_p
|| ((current_sched_info->flags & SCHED_RGN)
&& IS_SPECULATIN_BRANCHY_CHECK_P (insn))
|| (current_sched_info->flags & SCHED_EBB));

seems to expect a true jump. Anyway, this doesn't matter anymore in our case.

The assert expects an insn that is associated with bb->succs edges (i.e.
an insn after moving which we should somehow adjust CFG to maintain
instruction stream and CFG coherent). As I understand a block with a
trapping load at the end will have edges to all places control can be
transfered to. In that case, if scheduler front end supports movement
of instruction that affect control flow, any such insn will be handled
in a universal way.

@@ -1585,8 +1585,10 @@ sched_analyze_insn (struct deps *deps, r

/* If this instruction can throw an exception, then moving it changes
where block boundaries fall. This is mighty confusing elsewhere.
- Therefore, prevent such an instruction from being moved. */
- if (can_throw_internal (insn))
+ Therefore, prevent such an instruction from being moved. Same for
+ non-jump instructions that define block boundaries. */
+ if (((CALL_P (insn) || JUMP_P (insn)) && can_throw_internal (insn))
+ || (NNJUMP_INSN_P (insn) && control_flow_insn_p (insn)))
reg_pending_barrier = MVE_BARRIER;

/* Add dependencies if a scheduling barrier was found. */
>This is better to fix in add_branch_dependences ().

Why would we schedule these instructions in EBB mode and not in RGN mode?

I'm sure what do you mean. RGN mode doesn't have support to move such
instructions - EBB mode does.

EBB will schedule the testcase:

Before scheduling:

(note:HI 2 0 8 NTE_INSN_DELETED)

;; Start of basic block 2, registers live: 6 [bp] 7 [sp]
(note:HI 8 2 34 2 [bb 2] NTE_INSN_BASIC_BLCK)

(insn/f 34 8 35 2 (set (mem:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A8])
(reg/f:SI 6 bp)) 28 {*pushsi2} (nil)
(nil))

(insn/f 35 34 36 2 (set (reg/f:SI 6 bp)
(reg/f:SI 7 sp)) 34 {*movsi_1} (nil)
(expr_list:REG_DEAD (reg/f:SI 6 bp)
(nil)))

(note 36 35 6 2 NTE_INSN_PRLGUE_END)

(note:HI 6 36 7 2 NTE_INSN_DELETED)

(note:HI 7 6 21 2 NTE_INSN_FUNCTIN_BEG)

(insn:HI 21 7 22 2 (trap_if (const_int 1 [0x1])
(const_int 6 [0x6])) 553 {trap} (nil)
(nil))
;; End of basic block 2, registers live:
7 [sp]

(barrier:HI 22 21 23)

(note:HI 23 22 33 NTE_INSN_FUNCTIN_END)

During scheduling:
1. The trap was moved before insn 35 because register bp is dead before
the trap and hence insn 35 is useless.
2. Basic block 3 was created to hold insn 35.

At the end of scheduling:

(note:HI 2 0 8 NTE_INSN_DELETED)

(note:HI 8 2 36 2 [bb 2] NTE_INSN_BASIC_BLCK)

(note 36 8 6 2 NTE_INSN_PRLGUE_END)

(note:HI 6 36 7 2 NTE_INSN_DELETED)

(note:HI 7 6 34 2 NTE_INSN_FUNCTIN_BEG)

(insn/f:TI 34 7 21 2 (set (mem:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A8])
(reg/f:SI 6 bp)) 28 {*pushsi2} (nil)
(nil))

(insn:TI 21 34 40 2 (trap_if (const_int 1 [0x1])
(const_int 6 [0x6])) 553 {trap} (nil)
(nil))

(note 40 21 35 3 [bb 3] NTE_INSN_BASIC_BLCK)

(insn/f 35 40 22 3 (set (reg/f:SI 6 bp)
(reg/f:SI 7 sp)) 34 {*movsi_1} (nil)
(expr_list:REG_DEAD (reg/f:SI 6 bp)
(nil)))

(barrier:HI 22 35 23)

(note:HI 23 22 33 NTE_INSN_FUNCTIN_END)

(note 33 23 0 NTE_INSN_DELETED)

After scheduling: Basic block 3 was deleted because it had no predecessors.

(note:HI 2 0 8 NTE_INSN_DELETED)

(note:HI 8 2 36 2 [bb 2] NTE_INSN_BASIC_BLCK)

(note 36 8 6 2 NTE_INSN_PRLGUE_END)

(note:HI 6 36 7 2 NTE_INSN_DELETED)

(note:HI 7 6 34 2 NTE_INSN_FUNCTIN_BEG)

(insn/f:TI 34 7 21 2 (set (mem:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A8])
(reg/f:SI 6 bp)) 28 {*pushsi2} (nil)
(nil))

(insn:TI 21 34 23 2 (trap_if (const_int 1 [0x1])
(const_int 6 [0x6])) 553 {trap} (nil)
(nil))

(note:HI 23 21 33 NTE_INSN_FUNCTIN_END)

(note 33 23 0 NTE_INSN_DELETED)

The *only* problem that I see is that we didn't moved the barrier
together with the trap and thus lost it while deleting bb3. That is one
of the issues to be fixed before sched-ebb will be fully usable as
sched2 pass.

The assert expects an insn that is associated with bb->succs edges (i.e.
an insn after moving which we should somehow adjust CFG to maintain
instruction stream and CFG coherent). As I understand a block with a
trapping load at the end will have edges to all places control can be
transfered to. In that case, if scheduler front end supports movement
of instruction that affect control flow, any such insn will be handled
in a universal way.

There are also noreturn calls and, now, unconditional traps; neither has
successor edges in the CFG.

I'm sure what do you mean. RGN mode doesn't have support to move such
instructions - EBB mode does.

Is sched_analyze_insn not used in both mode? If so, do you mean that

/* If this instruction can throw an exception, then moving it changes
where block boundaries fall. This is mighty confusing elsewhere.
Therefore, prevent such an instruction from being moved. */

is totally useless in EBB mode?

Eric Botcazou wrote:
>The assert expects an insn that is associated with bb->succs edges (i.e.
>an insn after moving which we should somehow adjust CFG to maintain
>instruction stream and CFG coherent). As I understand a block with a
>trapping load at the end will have edges to all places control can be
>transfered to. In that case, if scheduler front end supports movement
>of instruction that affect control flow, any such insn will be handled
>in a universal way.

There are also noreturn calls and, now, unconditional traps; neither has
successor edges in the CFG.

Noreturn calls and unconditional traps also fall into this scheme as
they have edges to all places control can be transfered to - in their
case control goes out of the function - which makes associated edgeset
empty.


>I'm sure what do you mean. RGN mode doesn't have support to move such
>instructions - EBB mode does.

Is sched_analyze_insn not used in both mode? If so, do you mean that

/* If this instruction can throw an exception, then moving it changes
where block boundaries fall. This is mighty confusing elsewhere.
Therefore, prevent such an instruction from being moved. */

is totally useless in EBB mode?

can_throw_internal () [as I understand it] is used to check for EH code
which the trap looks to be not. Hence I'm not sure why it should be
handled as an EH insn. Answering your question: "I don't know. May be
EBB mode can live without it, nobody checked."

the other hand the trap is certainly a control flow insn and - while
for some reason parts of dependence handling code that are responsible
for handling jumps were written outside of sched-deps.c - things should
be kept in one place.

can_throw_internal () [as I understand it] is used to check for EH code
which the trap looks to be not. Hence I'm not sure why it should be
handled as an EH insn.

The comment seems pretty clear: "then moving it changes where block boundaries
fall. This is mighty confusing elsewhere."

Answering your question: "I don't know. May be EBB mode can live without
it, nobody checked."

K, let's play safe and assume that it's still useful in EBB mode somehow.

the other hand the trap is certainly a control flow insn and - while
for some reason parts of dependence handling code that are responsible
for handling jumps were written outside of sched-deps.c - things should
be kept in one place.

That's why I still think that sched_analyze_insn is the best place, as it
works for both EBB and RGN mode. I've added a comment trying to reflect
your position and installed the patch on both branches.

Thanks for your feedback.