<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;">On Feb 14, 2014, at 2:06 PM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="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;">On Thu, Feb 13, 2014 at 09:39:52AM -0800, Quentin Colombet wrote:<br><blockquote type="cite">Hi Tom,<br>On Feb 13, 2014, at 8:22 AM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><br><blockquote type="cite">On Wed, Feb 12, 2014 at 02:55:57PM -0800, Quentin Colombet wrote:<br><blockquote type="cite"><br>On Feb 12, 2014, at 2:07 PM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><br><blockquote type="cite">On Wed, Feb 12, 2014 at 01:43:25PM -0800, Quentin Colombet wrote:<br><blockquote type="cite">Hi Tom,<br><br>I think you find a bug in TargetLoweringBase::isLegalAddressingMode.<br><br>Indeed, TargetLoweringBase::isLegalAddressingMode (which is what it is called in your case), says that 3*r + r is a valid addressing mode.<br>Basically, this returns true for every scale that is not 0, 1, and 2. I believe it misses a default case in the switch statement that return false.<br><br>Alternatively, you could have overloaded this function for your target, to be sure it accepts only what it should.<br><br>Assuming you fixes this problem, the promotion would still be performed because in that case the transformation is neutral (i.e., we do not create new instructions).<br>The difference is: instead of selecting:<br>%in + %sext<br>we would select:<br>%in + %promoted_mul<br><br>That said, the mul 64bit is not a legal operation on your platform (nor is mul i32), thus, I could drop the change if we did not fold the instruction into the addressing mode when the operation is not legal.<br><br></blockquote><br>I think it makes sense to avoid the promotion if the result can't be<br>folded into the addressing mode.<br></blockquote>Well, that is a bit more complex than this.<br>When it is neutral, it may still be a good idea to perform the transformation because it may expose more simplifications (like sext + load).<br>However, I would prefer the promotion not to be smart about that, at least for now.<br><br>Therefore, I still think that checking for an operation to be legal or not may still be a viable approach.<br><br><br><blockquote type="cite">How will you determine whether or not an<br>operation is legal?<br></blockquote>TLI.isOperationLegalOrCustom?<br><br></blockquote><br>Ok, I didn't realize IR passes had access to this.<br><br><blockquote type="cite">See the attached patch for an example of implementation.<br><br>If this serves your purposes, let me know when you fixed the isLegalAddressingMode and I will commit this patch with your test case.<br><br></blockquote><br>I just sent a patch to the list to fix isLegalAddressingMode. I<br>included the test case with that patch and marked it as XFAIL. With<br>this patch and your patch everything works fine.<br></blockquote>Great!<br>I’ll wait for that patch to land before including this one.<br><br></blockquote><br>I just committed this patch. Make sure to remove XFAIL from the test case when you<br>commit yours. Thanks for fixing this.<br></div></blockquote><div>Done in r201439.</div><div><br></div><div>Thanks,</div><div>-Quentin</div><br><blockquote type="cite"><div style="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>-Tom<br><br><blockquote type="cite">Thanks,<br>-Quentin<br><br><blockquote type="cite"><br>Thanks,<br>Tom<br><br><blockquote type="cite">As it is the matched thinks the mul is folded anyway, making the proposed tuning useless.<br><br>Thanks,<br>-Quentin<br><br><blockquote type="cite"><br>-Tom<br><br><blockquote type="cite">Thanks for the test case.<br>-Quentin<br><br>On Feb 12, 2014, at 12:58 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br><blockquote type="cite">Sure, I’m looking into it.<br><br>-Quentin<br><br>On Feb 12, 2014, at 12:03 PM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><br><blockquote type="cite">Hi Quentin,<br><br>I've discovered a regression with this commit, please see the attached test case.<br>In this case, CodeGenPrepare is promoting the mul in a sext + mul pattern even<br>though the addressing mode it is creating isn't legal.<br><br>One interesting thing about this test case is that if you remove the<br>nsw from the mul instruction, then the incorrect transform does not<br>take place. I'm not sure why this matters. Would you mind taking a look?<br><br>Thanks,<br>Tom<br><codegen-prepare-addrmode-sext.ll><br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</blockquote></blockquote></blockquote></blockquote></blockquote></blockquote></div></blockquote></div><br></body></html>