<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Interesting, you’re right we are using these conversion as cast.</div><div class="">I think the LLVM language reference could be more clear.</div><div class="">In "If the value won’t fit in the floating point type, the results are undefined”, I understand “fit” means “fit exactly”. </div><div class="">I’d rather read as in the C++ standard "If the value being converted is outside the range of values that can be represented, the behavior is undefined”. They even felt necessary to add that "Loss of precision occurs if the integral value cannot be represented exactly as a value of the floating type"</div><div class=""><br class=""></div><div class="">Anyway, I think we can still exploit the fptosi/fptoui guarantee to replace:</div><div class=""><br class=""></div><div class=""> %fp = sitofp i25 %value to float</div><div class=""> %end = fptosi float %fp to i8</div><div class=""><br class=""></div><div class="">with </div><div class=""> </div><div class=""> %end = trunc i25 %value to %i8</div><div class=""><br class=""></div><div class="">Don’t you think?</div><div class=""><br class=""></div><div class="">I also haven’t seen any answer to my previous email comments:</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><div class=""><div class="">+ /* extra bit for sign */</div><div class="">+ (int)SrcTy->getScalarSizeInBits() < OpITy->getFPMantissaWidth()) {</div></div><div class=""><br class=""></div>It is present in the existing code, but I’m not sure I understand.<div class="">First, I don’t understand the strict < instead of <=.</div><div class="">Then I understand that SIToFPInst has one less bit to be considered because the sign bit wouldn’t be part of the mantissa, and it cannot contribute to a value here (the comment explains why) anyway.</div><div class="">I would have written something like:</div><div class=""><br class=""></div><div class=""> bool isSigned = isa<SIToFPInst>(OpI);</div><div class=""><div class=""> if ((isa<UIToFPInst>(OpI) || isSigned) &&</div></div><div class=""><div class=""> (int)FI.getType()->getScalarSizeInBits() - isSigned <= /*extra bit for sign */</div><div class=""> OpI->getType()->getFPMantissaWidth())</div></div><div class=""><br class=""></div><div class=""> It would be nice to also have the test that covers the edge case, which are I believe: for float i25, i24 and for double i53 and i54.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">+ if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())</div><div class="">+ return new ZExtInst(OpI->getOperand(0), FITy);</div><div class=""><br class=""></div><div class="">No need for ReplaceInstUsesWith() in the return statement? As in the previous branch of the if?</div></div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Thanks</div><div class=""><br class=""></div><div class="">— </div><div class="">Mehdi</div><div class=""><br class=""></div><div class=""><br class=""></div><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 11, 2015, at 7:52 AM, Fiona Glaser <<a href="mailto:fglaser@apple.com" class="">fglaser@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Um, this sounds very wrong.<div class=""><br class=""></div><div class="">The mantissa width is all about whether the float conversion causes rounding error, not about whether the value fits in the destination type. gcc doesn’t optimize out (int)(float)(int) for example, because that’s not legal.</div><div class=""><br class=""></div><div class="">Fiona</div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Feb 10, 2015, at 9:11 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">The discussion ended up with: fptoui(), fptosi(), sitofp(), and uitofp() are all undefined if the value cannot fit in the destination type.<div class=""><br class=""></div><div class="">So I’d expect we go for the simple solution where we don’t care about getFPMantissaWidth().</div><div class="">We only check getScalarSizeInBits() to insert the appropriate trunc or s/zext if necessary.</div><div class=""><br class=""></div><div class="">I assume that ISD::FP_TO_SINT (same with the other) has the same semantic as the IR instruction, and thus is applies to the DAG as well.</div><div class=""><br class=""></div><div class="">— </div><div class="">Mehdi</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Feb 10, 2015, at 6:35 PM, Fiona Glaser <<a href="mailto:fglaser@apple.com" class="">fglaser@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Any comments on the rest of this?<div class=""><br class=""></div><div class="">Fiona</div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Feb 9, 2015, at 4:35 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" class="">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Feel free to commit the change to port a test to FileCheck separately (and any time you'd like).<div class=""><br class=""></div><div class="">Thanks!</div><div class=""><br class=""></div><div class="">-eric<br class=""><br class=""><div class="gmail_quote">On Mon Feb 09 2015 at 2:53:45 PM Fiona Glaser <<a href="mailto:fglaser@apple.com" class="">fglaser@apple.com</a>> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ah, sorry, missed the comments about the tests. Updated the DAG patch with edited and improved test and minor code fixes.<br class="">
<br class="">
Fiona<br class="">
<br class="">
______________________________<u class=""></u>_________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class="">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/<u class=""></u>mailman/listinfo/llvm-commits</a><br class="">
</blockquote></div></div></div>
</div></blockquote></div><br class=""></div></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class=""></div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br class=""></body></html>