<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;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 9, 2015, at 2:26 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">For the instcombine part:</div><div class=""><br class=""></div><div class="">+ if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())</div><div class="">Shouldn’t we have some checks on the DstTy as well?</div><div class=""><br class=""></div><div class="">e.g., what happens with:</div><div class="">sitofp i32 to float</div><div class="">fptoui float to i8</div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">We shouldn’t be able to simplify that with a s/zext, do we… Or maybe we can because it is undef??</div></div></div></blockquote><div><br class=""></div><div><div>First not all i32 can fit in a float, it is checked in the outer enclosing test.</div><div>Let’s consider instead:</div><div><br class=""></div><div><div><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class="">sitofp i16 to float<br class="">fptoui float to i8<br class=""><br class=""></div></div></div></div><div>The result is defined only for input value that will fit in the i8 (Language Ref: " If the value cannot fit in ty2, the results are undefined.).</div><div>If that mean that we are allowed to do the transform, then we have to generate a trunc right?</div><div><br class=""></div><div>It means also that we don’t really care checking that the input integer size can be represented in the floating point type since the same language reference quote applies, i.e. :</div><div><br class=""></div><div>sitofp i128 to float<br class="">fptoso float to i128<br class=""></div><div><br class=""></div><div>can be turned into a no-op.</div><div><br class=""></div><div>Chandler or someone else familiar with the details of InstCombine might be able to shed some light on what is / isn’t allowed to do when the language ref. talk about “undefined” results. </div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div><div class=""><br class=""></div></div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><div class="">+ if (SrcTy == FITy)</div><div class="">+ return ReplaceInstUsesWith(FI, OpI->getOperand(0));</div><div class="">+ else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())</div><div class="">+ return new ZExtInst(OpI->getOperand(0), FITy);</div></div><div class=""><br class=""></div><div class="">No else after return.</div><div class=""><br class=""></div><div class=""><div class="">+ if (SrcTy == FITy)</div><div class="">+ return ReplaceInstUsesWith(FI, OpI->getOperand(0));</div><div class="">+ else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits()) {</div><div class=""><br class=""></div><div class="">Ditto.</div><div class=""><br class=""></div><div class="">+ if (isa<SIToFPInst>(OpI))</div><div class="">+ return new SExtInst(OpI->getOperand(0), FITy);</div><div class="">+ else</div><div class="">+ return new ZExtInst(OpI->getOperand(0), FITy);</div></div><div class="">Ditto</div><div class=""><br class=""></div><div class="">Could you increase the coverage in the tests?</div><div class="">1. You miss the input with sitofp.</div><div class="">2. Add some tests with multiple uses.</div><div class=""><br class=""></div><div class="">Also, please check what we actually generate the s/zext or nothing.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">For the DAG combine part:</div><div class=""><br class=""></div><div class=""><div class="">+ else if (VT.getScalarSizeInBits() > SrcVT.getScalarSizeInBits())</div><div class="">+ return DAG.getNode(Signed ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,</div><div class="">+ </div></div><div class="">No else after return.</div><div class=""><br class=""></div><div class=""><div class="">+ // We can only fold away the float conversion if the input range can be</div><div class="">+ // represented exactly in the float range.</div><div class="">+ if (APFloat::semanticsPrecision(sem) >= RequiredPrecision) </div></div><div class="">Same question as the inst combine part, any check on the destination type?</div><div class=""><br class=""></div><div class="">Same remarks on the tests.</div><div class=""><br class=""></div><div class="">Q.</div><br class=""><div class=""><div class="">On Feb 9, 2015, at 1:59 PM, Fiona Glaser <<a href="mailto:fglaser@apple.com" class="">fglaser@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><div style="font-family: Helvetica; font-size: 12px; 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;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Feb 9, 2015, at 1:47 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br class=""><div class=""><div class="">On Feb 9, 2015, at 1:44 PM, Fiona Glaser <<a href="mailto:fglaser@apple.com" class="">fglaser@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; 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;"><div class="" style="font-family: Helvetica; font-size: 12px; 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;"><div class=""><br class=""></div><div class="">Target-specific expansion or combine can end-up with this situation when it was not catched by ints-combine.</div><div class=""><div class=""><br class=""></div><div class="">So I believe we want it to be done *both* in inst-combine *and* in the DAG.</div></div></div></blockquote><div class="" style="font-family: Helvetica; font-size: 12px; 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 class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; 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;">I agree. That is also what I meant in my initial message.</div><div class="" style="font-family: Helvetica; font-size: 12px; 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 class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; 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;">Cheers,</div><div class="" style="font-family: Helvetica; font-size: 12px; 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;">Q.</div></div></blockquote><br class=""></div><div class="">Okay, should I merge the two patches then (and maybe include both tests)?</div></div></blockquote><div class=""><br class=""></div><div class="">Definitely include both tests :), but I’d rather have two different patches.</div></div></div></div></blockquote><br class=""></div><div style="font-family: Helvetica; font-size: 12px; 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;" class="">In that case, are the last versions of each of the patches in this thread (the DAG and instcombine ones) okay?</div><div style="font-family: Helvetica; font-size: 12px; 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;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; 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;" class="">Fiona</div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></body></html>