On 19.10.2010 17:57, Jie Zhang wrote:
Removing the failing assert fixes the test case. But I wonder why not just
get max_issue correct. I'm testing the attached patch. IMHO, max_issue
looks confusing.
* The concept of ISSUE POINT has never been used since the code landed in
repository.
* In the comment just before the function, it's mentioned that MAX_POINTS
is the sum of points of all instructions in READY. But it does not match
the code. The code only summarizes the points of the first MORE_ISSUE
instructions. If later ISSUE_POINTS become not uniform, that piece of code
should be redesigned.
So I think it's good to remove it now. And "top - choice_stack" is a good
replacement for top->n. So we can remove field n from struct choice_entry,
too.
Now I'm looking at MIPS target to find out why this change in the would
cause PR37360.
I agree that ISSUE_POINTS can be removed, as it was not used (maybe Maxim
can comment more on this). However, the assert is not about the points but
exactly about the situation when a target is lying to the compiler about
its issue rate.
The ideal situation is that we agree on that this should never happen, but
then you need to fix all targets that use this trick, and it seems that
there is at least mips, ppc, and x86-64 (which is why I pointed you to
45352). The fix would be to find out why claiming the true issue rate
degrades performance and to implement the proper scheduling hooks for
changing priority of some insns, or to enable -fsched-pressure for the
offending targets.
This is a lot of work, which is why this assert was installed in max_issue
for relatively short amount of time. Maybe it's time to try again, but
let's have a consensus first that this assert should never trigger by
design and we have enough flexibility in the scheduler to provide legal
means to achieve the same performance effect.
Andrey
/* ??? We used to assert here that we never issue more insns than issue_rate.
However, some targets (e.g. MIPS/SB1) claim lower issue rate than can be
achieved to get better performance. Until these targets are fixed to use
scheduler hooks to manipulate insns priority instead, the assert should
- be disabled.
-
- gcc_assert (more_issue >= 0); */
+ be disabled. */
