<html dir="ltr"><head></head><body style="text-align:left; direction:ltr;"><div>On Mon, 2018-12-10 at 10:55 -0600, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div>Trying to reduce the discussion to one e-mail thread and I picked this one. :-)</div><div><br></div><div>TBH, I'm not sure what the right solution is. What I do know is that the special cases are starting to get nuts. I see a three possible options:</div><div><br></div><div> 1. Unify the current brw_fs_nir code. This could happen a couple of ways:<br></div><div> a. Break the workarounds out into some helpers.</div></div></blockquote><div><br></div><div>I think this would make sense in any case.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div> b. Have a mega convert block (or function) that just handles everything in a general way</div></div></blockquote><div><br></div><div>Considering all the complexity that the conversion code is getting having a dedicated function probably makes more sense than having the logic merged in the giant switch statement of the NIR translator.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div> 2. Lower the conversions to multiple conversions in NIR. This would let us handle some of the issues such as DF -> F -> HF but not the struding issues.</div></div></blockquote><div><br></div><div>I see this as a nice thing to have. It is only a few cases but it will help recducing complexity in the backend.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div> 3. Just emit the conversion and handle all these cases in lower_conversions.</div></div></blockquote><div><br></div><div>Sounds a bit like the mega convert function from 1), only that we would be calling it from a different place.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div>I kind-of think option 3 may be the best but I'm not at all sure on that point and I think I'd be open to any of the above.</div></div></blockquote><div><br></div><div>Ideally I think we would do most of the above. 1.b) and/or 3) look like the only ones that would really make things better, but if we are going to do that we might as well put a bit more effort into it an the other things as well. They all look nice to have to me.</div><div><br></div><div>One thing about moving to lower conversions is that the pass happens very late and right now we are handling all these cases very early. I am not sure if this would have any actual implications but it is something we should verify before we commit to that, so I guess we might want to have the master conversion function first and design it so we can call that from lower_conversions or from the NIR translator and then expriment with both scenarios.</div><div><br></div><div>Iago</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div>--Jason<br></div><div><br><div class="gmail_quote"><div dir="ltr">On Mon, Dec 10, 2018 at 2:36 AM Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div style="text-align:left;direction:ltr"><div>Yes that might be a good thing to do. I mean, handling it in the backend isn't too bad if it was only this, but we now have a lot of conversion conversion cases and different kinds of restrictions on different gens and things start getting a bit messy. If we can pull some of these to NIR I think that at the very least that would make the backend code a bit more reasonable, which is a good thing.</div><div><br></div><div>Would you prefer that I do that before we land this or is it okay if I fix it up aftwewards?</div><div><br></div><div><div>In the backend I was also thinking about maybe pulling some of the conversion logic into one of more specialized conversion helpers that deal with specific things, maybe conversions from integers, or conversions from/to larger or smalller bit-sizes, etc I would need to think if there is one particular way to group conversions that makes things more sane overall.</div><div><br></div></div><div>Iago</div><div><br></div><div>On Fri, 2018-12-07 at 09:34 -0600, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr">Now I'm wondering even more about my previous question about just splitting it into two instructions in NIR.<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 4, 2018 at 1:18 AM Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">From: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>><br>
<br>
It is not supported directly in the HW, we need to convert to a 32-bit<br>
type first as intermediate step.<br>
<br>
v2 (Iago): handle conversions from 64-bit integers as well<br>
<br>
Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>><br>
---<br>
src/intel/compiler/brw_fs_nir.cpp | 42 ++++++++++++++++++++++++++++---<br>
1 file changed, 39 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp<br>
index 7294f49ddc0..9f3d3bf9762 100644<br>
--- a/src/intel/compiler/brw_fs_nir.cpp<br>
+++ b/src/intel/compiler/brw_fs_nir.cpp<br>
@@ -784,6 +784,19 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)<br>
*/<br>
<br>
case nir_op_f2f16:<br>
+ /* BDW PRM, vol02, Command Reference Instructions, mov - MOVE:<br>
+ *<br>
+ * "There is no direct conversion from HF to DF or DF to HF.<br>
+ * Use two instructions and F (Float) as an intermediate type.<br>
+ */<br>
+ if (nir_src_bit_size(instr->src[0].src) == 64) {<br>
+ fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_F, 1);<br>
+ inst = bld.MOV(tmp, op[0]);<br>
+ inst->saturate = instr->dest.saturate;<br>
+ inst = bld.MOV(result, tmp);<br>
+ inst->saturate = instr->dest.saturate;<br></blockquote><div><br></div><div>I don't think you need both saturates. Just the first one should be sufficient as it clamps to [0, 1] which is representable in 16-bit float.<br></div><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ break;<br>
+ }<br>
inst = bld.MOV(result, op[0]);<br>
inst->saturate = instr->dest.saturate;<br>
break;<br>
@@ -864,7 +877,32 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)<br>
inst->saturate = instr->dest.saturate;<br>
break;<br>
}<br>
- /* fallthrough */<br>
+ inst = bld.MOV(result, op[0]);<br>
+ inst->saturate = instr->dest.saturate;<br>
+ break;<br>
+<br>
+ case nir_op_i2f16:<br>
+ case nir_op_u2f16:<br>
+ /* BDW PRM, vol02, Command Reference Instructions, mov - MOVE:<br>
+ *<br>
+ * "There is no direct conversion from HF to Q/UQ or Q/UQ to HF.<br>
+ * Use two instructions and F (Float) or a word integer type or a<br>
+ * DWord integer type as an intermediate type."<br>
+ */<br>
+ if (nir_src_bit_size(instr->src[0].src) == 64) {<br>
+ brw_reg_type reg_type = instr->op == nir_op_i2f16 ?<br>
+ BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_UD;<br>
+ fs_reg tmp = bld.vgrf(reg_type, 1);<br>
+ inst = bld.MOV(tmp, op[0]);<br>
+ inst->saturate = instr->dest.saturate;<br>
+ inst = bld.MOV(result, tmp);<br>
+ inst->saturate = instr->dest.saturate;<br>
+ break;<br>
+ }<br>
+ inst = bld.MOV(result, op[0]);<br>
+ inst->saturate = instr->dest.saturate;<br>
+ break;<br>
+<br>
case nir_op_f2f32:<br>
case nir_op_f2i32:<br>
case nir_op_f2u32:<br>
@@ -874,8 +912,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)<br>
case nir_op_u2u32:<br>
case nir_op_i2i16:<br>
case nir_op_u2u16:<br>
- case nir_op_i2f16:<br>
- case nir_op_u2f16:<br>
case nir_op_i2i8:<br>
case nir_op_u2u8:<br>
inst = bld.MOV(result, op[0]);<br>
</blockquote></div></div></blockquote></div>
</blockquote></div></div></div>
</blockquote></body></html>