On Wed, Apr 15, 2015 at 6:44 PM, Matt Turner <matts...@gmail.com> wrote: > On Wed, Apr 15, 2015 at 5:13 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: >> On Wed, Apr 15, 2015 at 1:31 PM, Matt Turner <matts...@gmail.com> wrote: >>> On Wed, Apr 1, 2015 at 6:19 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: >>>> Instead of the complicated and broken-by-design pile of heuristics we had >>>> before, we now have a straightforward lowering: >>>> >>>> 1) All header sources are copied directly using force_writemask_all and, >>>> since they are guaranteed to be a single register, there are no >>>> force_sechalf issues. >>>> >>>> 2) All non-header sources are copied using the exact same force_sechalf >>>> and saturate modifiers as the LOAD_PAYLOAD operation itself. >>> >>> Let's not do this. Nothing puts a saturate modifier on LOAD_PAYLOAD >>> today, and it is kind of confusing about what it means. Can't we have >>> fbwrites that write depth as well. I wouldn't think we wanted to >>> saturate that. >> >> Sure. I can drop saturate and just assert that it's not set. We do >> want to keep force_sechalf and force_writemask_all though. > > I didn't think about those before, but I don't know how a load_payload > could have force_writemask_all set. Have I missed something?
No, no one (to my knowlege) sets force_writemask_all on it but I see no reason why it shouldn't be respected. As for saturate, we do for fb_writes when key->clamp_fragment_color is set. --Jason > I see that setup_color_payload sets force_sechalf for dual-source > fbwrites -- that's the only case we're going to have force_sechalf > set, right? > > That is, the Gen < 6 case is going to be handled by passing 16-channel > sources to load_payload and letting it do compr4? > >>> I don't think it buys us anything. If we just run copy propagation >>> after lower_load_payload() we'll get the code we want. > [snip] >>>> + /* The COMPR4 code took care of the first 4 sources. We'll >>>> let >>>> + * the regular path handle any remaining sources. Yes, we are >>>> + * modifying the instruction but we're about to delete it so >>>> + * this really doesn't hurt anything. >>>> + */ >>>> + inst->header_size += 4; >>> >>> I mean, while the comment is a true statement, why is doing this any >>> better than just... >>> >>>> + } >>>> + >>>> + for (uint8_t i = inst->header_size; i < inst->sources; i++) { >>> >>> ... changing this to inst->header_size + 4? >> >> Because the inst->header_size += 4 is predicated on it being a COMPR4 >> destination while the code below handles both the remaining sources >> (in the COMPR4 case) and the regular non-COMPR4 case. > > Ahh, right. It'd be fewer lines (no commenting necessary) to just have > a 'start' variable that you set to header_size at the top and +=4 > here. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev