<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;">LGTM after fix Jim's concerns.<div><br></div><div>Evan<br><div><div>On Aug 27, 2013, at 12:12 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Nit:<div><br></div><div><div>+def jtGPR : RegisterClass<"ARM", [i32], 32, (add (sequence "R%u", 0, 12))>;</div><div><br></div><div>You can define this like rGPR does, in terms of GPR (or rGPR) and a “sub” operator to get a bit more clarity about what’s going on. Something like:</div><div><div><br></div><div>def jtGPR : RegisterClass<"ARM", [i32], 32, (sub rGPR, LR)>;</div></div><div><br></div><div>Also please add a comment on the register class describing its purpose. The mispredict issue, as opposed to a correctness issue, is a bit subtle and if it’s not explicitly documented, it’ll be quickly forgotten why this register class is there in the first place.</div><div><br></div><div>-Jim</div><div><br></div><div><div>On Aug 27, 2013, at 3:17 AM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Hi Daniel,<div><br></div><div>The patch looks good to me, though I'd make sure Evan agrees before committing.</div><div><br></div><div>Would also be nice to get a test, if possible.</div><div><br></div><div>cheers,</div><div>--renato</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 14 August 2013 16:26, Daniel Stewart<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:stewartd@codeaurora.org" target="_blank">stewartd@codeaurora.org</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div lang="EN-US" link="blue" vlink="purple"><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Just wanted to bump this request again to see if I can get this patch reviewed. The explanation of the patch is below.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Thank you,<u></u><u></u></span></p><div class="im"><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Daniel Stewart<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">--<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Qualcomm Innovation Center, Inc is a member of Code Aurora Forum, hosted by The Linux Foundation<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"><u></u> <u></u></span></p></div><div><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0in 0in;"><p class="MsoNormal"><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;"><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a><span class="Apple-converted-space"> </span>[mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>]<span class="Apple-converted-space"> </span><b>On Behalf Of<span class="Apple-converted-space"> </span></b>Daniel Stewart<br><b>Sent:</b><span class="Apple-converted-space"> </span>Friday, August 09, 2013 10:24 AM<br><b>To:</b><span class="Apple-converted-space"> </span>'Evan Cheng'<br><b>Cc:</b><span class="Apple-converted-space"> </span>'Commits'<br><b>Subject:</b><span class="Apple-converted-space"> </span>RE: [PATCH] Added a new register class for Thumb PC-rel loads<u></u><u></u></span></p></div></div><div><div class="h5"><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">Yes, let me expand a bit on the patch. LLVM currently allows Thumb2 code to generate jump tables which use all rGPRs, including the link register. So it is possible to get code that looks like the following (taken from an actual compilation). This code is employing a jump table, where the jump is performed by the mov pc, lr (highlighted in red below).<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MLA r1,r4,r0,r9<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">ADDS r2,r4,#1<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">CMP r0,#8<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDRSH lr,[r3,r1,LSL #1]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDRSH r6,[r11],#2<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">SMLABB r8,lr,r6,r8<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">BCC.W 0x9e82 ; matrix_mul_matrix + 442<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B 0x9dfe ; matrix_mul_matrix + 310<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">ADR r2,{pc}+0x28 ; 0x9d7a<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">ADD r1,r2,r10,LSL #2<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDR r11,[sp,#0x20]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">STR r1,[sp,#0x18]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r1,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDR lr,[sp,#0x18]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">STR r1,[sp,#0x24]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOV r8,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r7,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r5,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r6,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r1,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r4,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">MOVS r2,#0<u></u><u></u></span></p><p class="MsoNormal"><b><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(149, 55, 53);">MOV pc,lr<u></u><u></u></span></b></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9dfe ; matrix_mul_matrix + 310<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9dfc ; matrix_mul_matrix + 308<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9de8 ; matrix_mul_matrix + 288<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9dd6 ; matrix_mul_matrix + 270<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9dc4 ; matrix_mul_matrix + 252<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9db2 ; matrix_mul_matrix + 234<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">B.W 0x9d96 ; matrix_mul_matrix + 206<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDR r4,[sp,#0x24]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">RSB r10,r4,#0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">AND r5,r10,r0<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">ADD r1,r5,r9<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: 'Courier New';">LDRSH r2,[r3,r1,LSL #1]<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">The mov pc, lr in this code causes the processor to believe it will be returning from the function it is in, rather than simply jumping within the function. The processor then mispredicts, causing a flush to occur. This patch seeks to remedy this situation be not allowing the lr register to be used as the index in a jump table for Thumb2 code. Therefore a mov pc, lr will not be generated for this case.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">The other part of this patch is fixing a hard-coded reference to a dynamically-generated variable. I discussed this issue earlier on the LLVMDev thread (topic line - [LLVMdev] Adding a new ARM RegisterClass). This fix is necessary for this particular patch because adding a new register class causes the dynamically generated classes used for GPRPairs to change. LLVM tries to find the correct intersections it needs to cover the space, and if a register class already covers the space, it won’t create a new class. In this example jtGPR covers the same space as rGPR from the standpoint of what is needed for the GPRPairs, as since jtGPR comes before rGPR alphabetically, the hard-coded reference to<span class="Apple-converted-space"> </span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif;">GPRPair with gsub_1 in rGPR is no longer created. In general, it seems preferable not to use hard-coded references to dynamically generated variables.<span class="Apple-converted-space"> </span><u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">Daniel<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">--<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">Qualcomm Innovation Center, Inc is a member of Code Aurora Forum, hosted by The Linux Foundation<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"><u></u> <u></u></span></p><div><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0in 0in;"><p class="MsoNormal"><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;"><span class="Apple-converted-space"> </span>Evan Cheng [<a href="mailto:evan.cheng@apple.com" target="_blank">mailto:evan.cheng@apple.com</a>]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, August 08, 2013 7:06 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Daniel Stewart<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>; Commits<br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH] Added a new register class for Thumb PC-rel loads<u></u><u></u></span></p></div></div><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">It would make it easier to review if you can illustrate the change with some assembly code sequence.<u></u><u></u></p><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Evan<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p><div><div><p class="MsoNormal">On Aug 8, 2013, at 7:30 AM, Daniel Stewart <<a href="mailto:stewartd@codeaurora.org" target="_blank">stewartd@codeaurora.org</a>> wrote:<u></u><u></u></p></div><p class="MsoNormal" style="margin-bottom: 12pt;"><u></u> <u></u></p><div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">This patch is for allowing a jump table (in Thumb mode) to not use the LR register, which can branch mispredicts during a mov lr, pc. If it could be reviewed, I’d appreciate it.<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"> <u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">This patch does the following:<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"> <u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">- Query for the GPRPair with gsub_1 in rGPR class.<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">This replaces the hard-coded reference<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"> <u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">- Add jtGPR register class to be used by load PC relative<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">address for jump table that avoids the use of LR register.<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">This prevents the codegen from generating mov pc, lr<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">which causes the target to think it is returning<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">from a function and predict that the next address will<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">be in the calling function.<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;"> <u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">Daniel Stewart<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">--<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-size: 11pt; font-family: Calibri, sans-serif;">Qualcomm Innovation Center, Inc is a member of Code Aurora Forum, hosted by The Linux Foundation<u></u><u></u></span></p></div><p class="MsoNormal"><span style="font-size: 9pt; font-family: Helvetica, sans-serif;"><0001-Added-register-class-with-no-LR-used-by-PC-rel-load.patch>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank"><span style="color: purple;">llvm-commits@cs.uiuc.edu</span></a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank"><span style="color: purple;">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</span></a><u></u><u></u></span></p></div></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div></div><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br><br></blockquote></div><br></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div></div></div></blockquote></div><br></div></body></html>