Re: New assert in haifa-sched.c
Yes, the assert is really checking exactly that. Several pieces of haifa-sched.c assume that the instruction has been recognized during scheduler initialization to speed up checking if instruction is normal or some kind of use/clobber/asm. What happens if an instruction hasn't been recognized upon reaching these parts of haifa-sched.c? Will it be only mis-scheduled, i.e. will this only result in inferior, but still correct code? If so, the assertion shouldn't be enabled in release mode but only if ENABLE_CHECKING is defined. -- Eric Botcazou
Re: New assert in haifa-sched.c
Eric Botcazou wrote: Yes, the assert is really checking exactly that. Several pieces of haifa-sched.c assume that the instruction has been recognized during scheduler initialization to speed up checking if instruction is normal or some kind of use/clobber/asm. What happens if an instruction hasn't been recognized upon reaching these parts of haifa-sched.c? Will it be only mis-scheduled, i.e. will this only result in inferior, but still correct code? The code will still be correct, but I think there's more than 1 assert relying on this property, so GCC is likely to ICE. If so, the assertion shouldn't be enabled in release mode but only if ENABLE_CHECKING is defined. I don't see the logic here. It is certainly not that hard to fix the code to make this assertion trivial, simple loop through all insns in sched_init() will do it. There is a bug and it should get fixed rather than suppressed. Or maybe I misunderstood you? -- Maxim
Re: New assert in haifa-sched.c
Eric Botcazou writes: Yes, the assert is really checking exactly that. Several pieces of haifa-sched.c assume that the instruction has been recognized during scheduler initialization to speed up checking if instruction is normal or some kind of use/clobber/asm. What happens if an instruction hasn't been recognized upon reaching these parts of haifa-sched.c? Will it be only mis-scheduled, i.e. will this only result in inferior, but still correct code? If so, the assertion shouldn't be enabled in release mode but only if ENABLE_CHECKING is defined. Well, I can't bootstrap as it is so it would be nice if this was fixed regardless of the checking level. (And this is what my patch does in http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00574.html.) Another option would be to completely remove the assert. Adam
Re: New assert in haifa-sched.c
I don't see the logic here. It is certainly not that hard to fix the code to make this assertion trivial, simple loop through all insns in sched_init() will do it. Yes, but apparently nobody is willing to do that at the moment so we'll need to kludge until then and add calls to recog_memoized when we stumble on insns that have not been recognized. But our laziness shouldn't impact the user if we are confident that this cannot result in wrong code. There is a bug and it should get fixed rather than suppressed. AFAICS nobody has submitted a real fix for this bug. I'm ready to approve Adam's patch but only if the assertion is downgraded to ENABLE_CHECKING. -- Eric Botcazou
Re: New assert in haifa-sched.c
Adam Nemet wrote: Maxim Kuvyrkov writes: I'm not 100% sure about current state of things, considering recent merge of sel-sched, but before that it was: set_priorities() - priority() - dep_cost() - recog_memoized(). I don't think that was the case for all insns even before the patch. The only new thing is the assert which now catches this. If the consumer is an asm just like in gcc.dg/tree-ssa/stdarg-2.c:f10() then we would not call recog on the producer inside dep_cost*. The patch below fixes the issue for me. I am going to test this if it looks good to people. I think a cleaner way would be to call recog_memoized() from within sched_init(). There's no ideal place to put call to recog_memoized() at the moment, but something like init_h_i_d() seems to be a better choice for initializing INSN_CODE() than dep_cost(). As far as your patch goes, I think it's ok (I'm not the maintainer, though). -- Maxim
Re: New assert in haifa-sched.c
Adam Nemet wrote:
haifa-sched.c:
2302/* Let the target filter the search space. */
2303for (i = 1; i ready-n_ready; i++)
2304 if (!ready_try[i])
2305{
2306 insn = ready_element (ready, i);
2307
2308 gcc_assert (INSN_CODE (insn) = 0
2309 || recog_memoized (insn) 0);
I am hitting this assert with the Octeon pipeline patch. Isn't the check on
recog_memoized reversed? Or are we really asserting here that the insn has
either been recognized earlier or won't be recognized now either?!
Yes, the assert is really checking exactly that. Several pieces of
haifa-sched.c assume that the instruction has been recognized during
scheduler initialization to speed up checking if instruction is normal
or some kind of use/clobber/asm.
As max_issue () is one of hot spots in scheduler (and compiler), not
calling recog_memoized() saves up some time.
If the assert is failing, then something is wrong at initialization stage.
--
Maxim
Re: New assert in haifa-sched.c
Maxim Kuvyrkov writes: Yes, the assert is really checking exactly that. Several pieces of haifa-sched.c assume that the instruction has been recognized during scheduler initialization to speed up checking if instruction is normal or some kind of use/clobber/asm. Thanks for the info but I can't seem to find the code where this is supposed to be happening. Can you point me to the code? Adam
Re: New assert in haifa-sched.c
Adam Nemet wrote: Maxim Kuvyrkov writes: Yes, the assert is really checking exactly that. Several pieces of haifa-sched.c assume that the instruction has been recognized during scheduler initialization to speed up checking if instruction is normal or some kind of use/clobber/asm. Thanks for the info but I can't seem to find the code where this is supposed to be happening. Can you point me to the code? I'm not 100% sure about current state of things, considering recent merge of sel-sched, but before that it was: set_priorities() - priority() - dep_cost() - recog_memoized(). -- Maxim
Re: New assert in haifa-sched.c
Maxim Kuvyrkov writes:
I'm not 100% sure about current state of things, considering recent
merge of sel-sched, but before that it was:
set_priorities() - priority() - dep_cost() - recog_memoized().
I don't think that was the case for all insns even before the patch. The only
new thing is the assert which now catches this.
If the consumer is an asm just like in gcc.dg/tree-ssa/stdarg-2.c:f10() then
we would not call recog on the producer inside dep_cost*.
The patch below fixes the issue for me. I am going to test this if it looks
good to people.
Adam
Index: haifa-sched.c
===
--- haifa-sched.c (revision 139918)
+++ haifa-sched.c (working copy)
@@ -646,7 +646,8 @@ insn_cost (rtx insn)
/* Compute cost of dependence LINK.
This is the number of cycles between instruction issue and
- instruction results. */
+ instruction results. We also use this function to call
+ recog_memoized on all insns. */
int
dep_cost_1 (dep_t link, dw_t dw)
{
@@ -657,7 +658,10 @@ dep_cost_1 (dep_t link, dw_t dw)
This allows the computation of a function's result and parameter
values to overlap the return and call. */
if (recog_memoized (used) 0)
-cost = 0;
+{
+ cost = 0;
+ recog_memoized (DEP_PRO (link));
+}
else
{
rtx insn = DEP_PRO (link);
@@ -2305,6 +2309,8 @@ choose_ready (struct ready_list *ready,
{
insn = ready_element (ready, i);
+ /* If this insn is recognizable we should have already
+ recognized it in dep_cost_1. */
gcc_assert (INSN_CODE (insn) = 0
|| recog_memoized (insn) 0);
