On 09/05/2012 11:16 PM, Eric Anholt wrote:
> This fixes the blue zombies bug in l4d2.
>
> NOTE: This is a candidate for the 9.0 branch.
> ---
> .../drivers/dri/i965/brw_fs_live_variables.cpp | 24
> ++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> index 16480ad..392691d 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> @@ -221,6 +221,30 @@ fs_visitor::calculate_live_intervals()
> }
>
> this->live_intervals_valid = true;
> +
> + /* Note in the non-control-flow code above, that we only take def[] as the
> + * first store, and use[] as the last use. We use this in dead code
> + * elimination, to determine when a store never gets used. However, we
> + * also use these arrays to answer the virtual_grf_interferes() question
> + * (live interval analysis), which is used for register coalescing and
> + * register allocation.
> + *
> + * So, there's a conflict over what the array should mean: if use[]
> + * considers a def after the last use, then the dead code elimination pass
> + * never does anything (and it's an important pass!). But if we don't
> + * include dead code, then virtual_grf_interferes() lies and we'll do
> + * horrible things like coalesce the register that is dead-code-written
> + * into another register that was live across the dead write (causing the
> + * use of the second register to take the dead write's source value
> instead
> + * of the coalesced MOV's source value).
> + *
> + * To resolve the conflict, immediately after calculating live intervals,
> + * detect dead code, nuke it, and if we changed anything, calculate again
> + * before returning to the caller. Now we happen to produce def[] and
> + * use[] arrays that will work for virtual_grf_interferes().
> + */
> + if (dead_code_eliminate())
> + calculate_live_intervals();
> }
>
> bool
I am worried that this will come back to haunt us later. This is the
first time calculate_live_intervals() has actually *changed* the IR,
rather than analyze it.
Specifically, if an optimization pass were to do:
fs_cfg cfg(this);
calculate_live_intervals();
and the start/end of a basic block happened to be dead code, then when
we hit
for (fs_inst *inst = block->start;
inst != block->end->next;
inst = (fs_inst *) inst->next) {
we'll most assuredly start traversing data and hit a NULL pointer (if we
don't crash when dereferencing block->start) since you'll never hit
block->end.
Granted, no code actually uses both CFGs and live intervals today, but
it sounds plausible.
Here's one idea for safeguarding against this:
1. Add a new "int using_cfg" variable to fs_visitor.
2. In the fs_cfg() constructor, increment v->using_cfg.
3. In the fs_cfg() destructor, decrement v->using_cfg.
4. In calculate_live_intervals, assert(!using_cfg);
Then it'd least die with a reasonable message.
What do you think? Worth doing? Something else? Am I being too paranoid?
Regardless of the paranoia I think you should push this (unless you
think of a better solution). After spending nearly a whole week hitting
my head against the wall, I'm ready to see this fixed.
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
