<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Feb 14, 2014, at 1:00 PM, Roland Scheidegger <<a href="mailto:sroland@vmware.com">sroland@vmware.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Am 14.02.2014 18:07, schrieb Jeff Muizelaar:<br><br>I'll need to take another look and run some tests, though I've got some<br>quick comments:<br><br><br>@@ -1031,16 +1082,28 @@ lp_build_sample_image_linear(struct<br>lp_build_sample_context *bld,<br> s = lp_build_mul_imm(&bld->coord_bld, s, 256);<br> if (dims >= 2)<br> t = lp_build_mul_imm(&bld->coord_bld, t, 256);<br> if (dims >= 3)<br> r = lp_build_mul_imm(&bld->coord_bld, r, 256);<br> }<br><br> /* convert float to int */<br>+ half = lp_build_const_vec(bld->gallivm, bld->coord_bld.type, 0.5);<br>+ s = lp_build_add(&bld->coord_bld, s, half);<br>+ s = LLVMBuildFPToSI(builder, s, i32_vec_type, "");<br>+ if (dims >= 2) {<br>+ t = lp_build_add(&bld->coord_bld, t, half);<br>+ t = LLVMBuildFPToSI(builder, t, i32_vec_type, "");<br>+ }<br>+ if (dims >= 3) {<br>+ r = lp_build_add(&bld->coord_bld, r, half);<br>+ r = LLVMBuildFPToSI(builder, r, i32_vec_type, "");<br>+ }<br>+<br> s = LLVMBuildFPToSI(builder, s, i32_vec_type, "");<br> if (dims >= 2)<br> t = LLVMBuildFPToSI(builder, t, i32_vec_type, "");<br> if (dims >= 3)<br> r = LLVMBuildFPToSI(builder, r, i32_vec_type, "");<br>This looks quite incorrect you're converting the s/t/r coords twice from<br>float to int.<br></div></blockquote><div><br></div><div>Yep. I forgot to remove this second hunk.</div><div><br></div><blockquote type="cite"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"> /* subtract 0.5 (add -128) */<br> i32_c128 = lp_build_const_int_vec(bld->gallivm, i32.type, -128);<br><br><br>Also, the add looks iffy as it won't work correctly if the coords are<br>negative, since the FPToSI is of course trunc, not floor.<br></div></blockquote><div><br></div><div>I think it will be ok because the REPEAT case avoids negative coord before converting to int and the other cases clamp to 0.</div><div><br></div><blockquote type="cite"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Maybe instead of using add + fptosi should just use lp_build_iround<br>(which is just one sse instruction too on x86 though if you're targeting<br>another arch it will definitely be more code at least unless someone<br>adds an intrinsic for it if the cpu even has one). Might not matter<br>though depending on address mode…</div></blockquote><div><br></div><div>Yeah, that might be a better idea.</div><br><blockquote type="cite"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>And I might be missing something why do you think the new repeat code is<br>faster? Though that might also depend on arch_rounding being available<br>and such but at first looks it seems slightly more complex to me.<br></div></blockquote><div><br></div><div>The current code converts integer and fractional parts to integer separately. It also does the subtract 0.5 in floating point instead of integer arithmetic (-128).</div><div><br></div><div>-Jeff</div></div></body></html>