<html><head></head><body><div>On Tue, 2017-10-31 at 07:20 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div>On Tue, Oct 31, 2017 at 12:01 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><div><div><div class="h5"><div>On Mon, 2017-10-30 at 11:29 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 30, 2017 at 12:15 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><div><div><div class="m_-5955697755301179068h5"><div>On Fri, 2017-10-27 at 12:21 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 26, 2017 at 11:53 PM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite">On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:<br>
> ---<br>
> src/intel/compiler/brw_fs_<wbr>nir.cpp | 33 +++++++++++++++++++++------<br>
<div><div class="m_-5955697755301179068m_681881548967073001h5">> ------<br>
> 1 file changed, 21 insertions(+), 12 deletions(-)<br>
><br>
> diff --git a/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> b/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> index e008e2e..a441f57 100644<br>
> --- a/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> +++ b/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> @@ -1441,11 +1441,19 @@ fs_visitor::get_nir_src(const nir_src &src)<br>
> <a href="http://src.reg.ba" target="_blank">src.reg.ba</a><wbr>se_offset * src.reg.reg-<br>
> >num_components);<br>
> }<br>
> <br>
> - /* to avoid floating-point denorm flushing problems, set the type<br>
> by<br>
> - * default to D - instructions that need floating point semantics<br>
> will set<br>
> - * this to F if they need to<br>
> - */<br>
> - return retype(reg, BRW_REGISTER_TYPE_D);<br>
> + if (nir_src_bit_size(src) == 64 && devinfo->gen == 7) {<br>
> + /* The only 64-bit type available on gen7 is DF, so use that.<br>
> */<br>
> + reg.type = BRW_REGISTER_TYPE_DF;<br>
> + } else {<br>
> + /* To avoid floating-point denorm flushing problems, set the<br>
> type by<br>
> + * default to an integer type - instructions that need<br>
> floating point<br>
> + * semantics will set this to F if they need to<br>
> + */<br>
> + reg.type = brw_reg_type_from_bit_size(nir<wbr>_src_bit_size(src),<br>
> + <wbr> BRW_REGISTER_TY<wbr>PE_D);<br>
> + }<br>
> +<br>
> + return reg;<br>
> }<br>
> <br>
> /**<br>
> @@ -1455,6 +1463,10 @@ fs_reg<br>
> fs_visitor::get_nir_src_imm(c<wbr>onst nir_src &src)<br>
> {<br>
> nir_const_value *val = nir_src_as_const_value(src);<br>
> + /* This function shouldn't be called on anything which can even<br>
> + * possibly be 64 bits as it can't do what it claims.<br>
> + */<br>
<br>
</div></div>What would be wrong with something like this?<br>
<br>
if (nir_src_bit_size(src) == 32)<br>
<span> return val ? fs_reg(brw_imm_d(val->i32[0])) : get_nir_src(src);<br>
</span>else<br>
return val ? fs_reg(brw_imm_df(val->f64[0])<wbr>) : get_nir_src(src);<br><div class="m_-5955697755301179068m_681881548967073001HOEnZb"><div class="m_-5955697755301179068m_681881548967073001h5"></div></div><br></blockquote><div><br></div><div>Because double immediates only really work on BDW+ and I didn't want someone to call this function and get tripped up by that.<br></div></div></div></div></blockquote><div><br></div></div></div><div>Ok, fair enough. In that case, maybe I'd suggest to clarify this in the comment, since otherwise it is a bit confusing (or maybe assert on the generation rather than the bitsize since that would be more self-explanatory).</div></div><br></blockquote><div><br></div><div>I'm not really clear on what you're asking for. Do you want it to work for 64-bit immediates and just assert on gen7? Or do you want the comment to be more clear?<br></div></div></div></div></blockquote><div><br></div></div></div><div>I think either of them would be fine. My issue is with the comment where it says that this cannot possibly work with 64-bit immediates in general, because that is not true for gen8+.</div><div>I am fine with a change in the comment clarifying that such thing would only work for gen8+ but we decide to only support 32-bit paths. I'd also be fine if we decided to support gen8+ and just assert for gen7 but I am not asking that you do that if you prefer to keep things 32-bit only for this.</div></div><br></blockquote><div><br></div><div>Alright, I'll update the comment. How about:</div><div><br></div>This function should not be called on any value which may be 64 bits. We could theoretically support 64-bit on gen8+ but we choose not to because it wouldn't work in general (no gen7 support) and there are enough restrictions in 64-bit immediates that you can't take the return value and treat it the same as the result of </div></div></div></blockquote><div><br></div><div>That looks good to me, thanks!</div><div><br></div><div>Iago</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div><div><div class="h5"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div><div><div class="m_-5955697755301179068h5"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote type="cite"><div class="m_-5955697755301179068m_681881548967073001HOEnZb"><div class="m_-5955697755301179068m_681881548967073001h5">
> + assert(nir_src_bit_size(sr<wbr>c) == 32);<br>
> return val ? fs_reg(brw_imm_d(val->i32[0])) : get_nir_src(src);<br>
> }<br>
> <br>
> @@ -2648,8 +2660,7 @@ fs_visitor::nir_emit_tcs_intri<wbr>nsic(const<br>
> fs_builder &bld,<br>
> */<br>
> unsigned channel = iter * 2 + i;<br>
> fs_reg dest = shuffle_64bit_data_for_32bit_w<wbr>rite(bld,<br>
> - retype(offs<wbr>et(value, bld, 2 * channel),<br>
> BRW_REGISTER_TYPE_DF),<br>
> - 1);<br>
> + offset(valu<wbr>e, bld, channel), 1);<br>
> <br>
> srcs[header_re<wbr>gs + (i + first_component) * 2] = dest;<br>
> srcs[header_re<wbr>gs + (i + first_component) * 2 + 1] =<br>
> @@ -3505,8 +3516,7 @@ fs_visitor::nir_emit_cs_intrin<wbr>sic(const<br>
> fs_builder &bld,<br>
> if (nir_src_bit_size(instr->src[0<wbr>]) == 64) {<br>
> type_size = 8;<br>
> val_reg = shuffle_64bit_data_for_32bit_w<wbr>rite(bld,<br>
> - retype(val_reg, BRW_REGISTER_TYPE_DF),<br>
> - instr->num_compon<wbr>ents);<br>
> + val_reg, instr->num_components);<br>
> }<br>
> <br>
> unsigned type_slots = type_size / 4;<br>
> @@ -4005,8 +4015,7 @@ fs_visitor::nir_emit_intrinsic<wbr>(const fs_builder<br>
> &bld, nir_intrinsic_instr *instr<br>
> if (nir_src_bit_size(instr->src[0<wbr>]) == 64) {<br>
> type_size = 8;<br>
> val_reg = shuffle_64bit_data_for_32bit_w<wbr>rite(bld,<br>
> - retype(val_reg, BRW_REGISTER_TYPE_DF),<br>
> - instr->num_compon<wbr>ents);<br>
> + val_reg, instr->num_components);<br>
> }<br>
> <br>
> unsigned type_slots = type_size / 4;<br>
> @@ -4063,7 +4072,7 @@ fs_visitor::nir_emit_intrinsic<wbr>(const fs_builder<br>
> &bld, nir_intrinsic_instr *instr<br>
> unsigned first_component = nir_intrinsic_component(instr)<wbr>;<br>
> if (nir_src_bit_size(instr->src[0<wbr>]) == 64) {<br>
> fs_reg tmp = shuffle_64bit_data_for_32bit_w<wbr>rite(bld,<br>
> - retype(src, BRW_REGISTER_TYPE_DF), num_components);<br>
> + src, num_components);<br>
> src = tmp;<br>
> num_components *= 2;<br>
> }<br>
</div></div><br></blockquote></div><br></div></div>
</blockquote></div></div></div><br></blockquote></div><br></div></div>
</blockquote></div></div></div><br></blockquote></div><br></div></div>
</blockquote></body></html>