<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;"><div><br></div><div>For the instcombine part:</div><div><br></div><div>+ if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())</div><div>Shouldn’t we have some checks on the DstTy as well?</div><div><br></div><div>e.g., what happens with:</div><div>sitofp i32 to float</div><div>fptoui float to i8</div><div><br></div><div>We shouldn’t be able to simplify that with a s/zext, do we… Or maybe we can because it is undef??</div><div><br></div><div><br></div><div><div>+ if (SrcTy == FITy)</div><div>+ return ReplaceInstUsesWith(FI, OpI->getOperand(0));</div><div>+ else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())</div><div>+ return new ZExtInst(OpI->getOperand(0), FITy);</div></div><div><br></div><div>No else after return.</div><div><br></div><div><div>+ if (SrcTy == FITy)</div><div>+ return ReplaceInstUsesWith(FI, OpI->getOperand(0));</div><div>+ else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits()) {</div><div><br></div><div>Ditto.</div><div><br></div><div>+ if (isa<SIToFPInst>(OpI))</div><div>+ return new SExtInst(OpI->getOperand(0), FITy);</div><div>+ else</div><div>+ return new ZExtInst(OpI->getOperand(0), FITy);</div></div><div>Ditto</div><div><br></div><div>Could you increase the coverage in the tests?</div><div>1. You miss the input with sitofp.</div><div>2. Add some tests with multiple uses.</div><div><br></div><div>Also, please check what we actually generate the s/zext or nothing.</div><div><br></div><div><br></div><div><br></div><div>For the DAG combine part:</div><div><br></div><div><div>+ else if (VT.getScalarSizeInBits() > SrcVT.getScalarSizeInBits())</div><div>+ return DAG.getNode(Signed ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,</div><div>+ </div></div><div>No else after return.</div><div><br></div><div><div>+ // We can only fold away the float conversion if the input range can be</div><div>+ // represented exactly in the float range.</div><div>+ if (APFloat::semanticsPrecision(sem) >= RequiredPrecision) </div></div><div>Same question as the inst combine part, any check on the destination type?</div><div><br></div><div>Same remarks on the tests.</div><div><br></div><div>Q.</div><br><div><div>On Feb 9, 2015, at 1:59 PM, Fiona Glaser <<a href="mailto:fglaser@apple.com">fglaser@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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;"><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;">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;"><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;">Fiona</div></blockquote></div><br></body></html>