[Mesa-dev] [PATCH 09/16] i965/fs: Apply conditional mod specially to split MAD/LRP.

[Matt Turner]

On Tue, Jan 20, 2015 at 12:32 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Monday, January 19, 2015 03:31:08 PM Matt Turner wrote:
>> Otherwise we'll apply the conditional mod to only one of SIMD8
>> instructions and trigger an assertion.
>>
>> NoDDClr/NoDDChk have the same problem but we never apply those to these
>> instructions, so I'm leaving them for a later time.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> index ab848f1..f35da71 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> @@ -1656,10 +1656,16 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>>        brw_set_default_access_mode(p, BRW_ALIGN_16);
>>           if (dispatch_width == 16 && brw->gen < 8 && !brw->is_haswell) {
>>           brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>> -         brw_MAD(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>> +            brw_inst *f = brw_MAD(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>>           brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
>> -         brw_MAD(p, sechalf(dst), sechalf(src[0]), sechalf(src[1]), sechalf(src[2]));
>> +            brw_inst *s = brw_MAD(p, sechalf(dst), sechalf(src[0]), sechalf(src[1]), sechalf(src[2]));
>>           brw_set_default_compression_control(p, BRW_COMPRESSION_COMPRESSED);
>> +
>> +            if (inst->conditional_mod) {
>> +               brw_inst_set_cond_modifier(brw, f, inst->conditional_mod);
>> +               brw_inst_set_cond_modifier(brw, s, inst->conditional_mod);
>> +               inst->conditional_mod = BRW_CONDITIONAL_NONE;
>
> Having the generator mutate the incoming IR feels dirty to me.  Honestly, it
> should be const...we've never changed it until now.
>
> I see what you're trying to accomplish - bypassing the assertion failure
> about conditional_mod set with more than one instruction.
>
> Maybe add a "bool multiple_instructions_allowed" flag, set it to false before
> the switch, set it true here, and check it later to skip the assert?  Seems
> ugly, but not as bad as mutating the IR.

Sure.

> I think a better solution (after this series lands!) would be to generate two
> MADs/LRPs at the fs_visitor level, and just emit a single instruction for each
> at the generator level.  We should have the infrastructure now and it'd let us
> schedule them.

I really don't think adding that complication is a good idea.