On Thu, Aug 7, 2014 at 7:55 AM, Pohjolainen, Topi <topi.pohjolai...@intel.com> wrote: > On Thu, Jul 24, 2014 at 07:54:20PM -0700, Matt Turner wrote: >> To avoid invalidating and recreating the control flow graph. Also stop >> invalidating the CFG in places we didn't add or remove an instruction. >> >> cfg calculations: 202951 -> 80307 (-60.43%) >> --- [snip] >> - invalidate_live_intervals(); >> + invalidate_live_intervals(false); >> calculate_live_intervals(); > > With a quick glance this looked a little odd, we don't invalidate but > calculate nevertheless. But we actually do invalidate, the intervals, but > not the cfg. One just needs to remember what the argument actually stands for.
Right, I'm looking forward to deleting the argument from invalidate_live_intervals(). I hate boolean arguments like this. >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp >> b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp >> index 390448a..ebee42f 100644 >> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp >> @@ -403,7 +403,7 @@ vec4_visitor::opt_copy_propagation() >> } >> >> if (progress) >> - invalidate_live_intervals(); >> + invalidate_live_intervals(false); > > The patch either explicitly changes an iteration to use blocks and block-aware > manipulation routines, or relies on the fact that the iteration already > operates with blocks. > This is the only occurence that initially looked to be using the flat > instruction list (even though the comment in the existing code says that it > operates on basic blocks). Closer inspection reveals that it actually doesn't > add or remove instructions, it just modifies existing, right? Indeed, exactly right. > Somebody else should have a look as well, but > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > > And just as a thought, splitting this to three parts might assist bisecting > later on. One patch just changing the cases where iteration already works > with blocks, another dealing with those cases where the iteration needs to be > modified as well and one patch dealing with the case here. Yes, probably a good idea. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
