<div dir="ltr">ARM lgtm</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 12, 2014 at 6:40 AM, Daniel Sanders <span dir="ltr"><<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Thanks. The MIPS part looks good to me.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Narayan Kamath [mailto:<a href="mailto:narayan@google.com" target="_blank">narayan@google.com</a>]
<br>
<b>Sent:</b> 12 March 2014 13:23</span></p><div><div class="h5"><br>
<b>To:</b> Daniel Sanders<br>
<b>Cc:</b> Elliott Hughes; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>; Jean-Luc Brouillet; JF Bastien; Renato Golin<br>
<b>Subject:</b> Re: [patch] implement __clear_cache for arm32 & mips<u></u><u></u></div></div><p></p>
</div>
</div><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">Apologies, I got caught up in the ARM changes. The attached patch<u></u><u></u></p>
<div>
<p class="MsoNormal">switches MIPS to BCACHE.<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Wed, Mar 12, 2014 at 11:58 AM, Daniel Sanders <<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">You haven't switched the ICACHE argument in the MIPS version to BCACHE yet. With that change the MIPS part will
LGTM.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Narayan
Kamath [mailto:<a href="mailto:narayan@google.com" target="_blank">narayan@google.com</a>]
<br>
<b>Sent:</b> 12 March 2014 09:25<br>
<b>To:</b> Daniel Sanders<br>
<b>Cc:</b> Elliott Hughes; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">
llvm-commits@cs.uiuc.edu</a>; Jean-Luc Brouillet; JF Bastien; Renato Golin</span><u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><br>
<b>Subject:</b> Re: [patch] implement __clear_cache for arm32 & mips<u></u><u></u></p>
</div>
</div>
</div>
</div>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal">Thanks for the reviews everyone.<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">After some consultation with engineers at ARM (David butcher et al.), we decided<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">that the switch to ARM mode to issue the supervisor call is unnecessary. It might be<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">that some ancient kernel had a bad svc handler that didn't correctly switch processor<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">state if it was thumb, but we can ensure that such kernels don't ship on android devices.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">I've attached the updated patch. Please take a look.<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"> <u></u><u></u></p>
<div>
<p class="MsoNormal">On Fri, Mar 7, 2014 at 3:08 PM, Daniel Sanders <<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">That makes sense to me. I've put a ticket about the better implementation into the LLVM bugzilla (PR19076) so we
don't forget it.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Narayan
Kamath [mailto:<a href="mailto:narayan@google.com" target="_blank">narayan@google.com</a>]
<br>
<b>Sent:</b> 07 March 2014 14:10<br>
<b>To:</b> Daniel Sanders; Elliott Hughes<br>
<b>Cc:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>; Jean-Luc Brouillet; JF Bastien; Renato Golin<br>
<b>Subject:</b> Re: [patch] implement __clear_cache for arm32 & mips</span><u></u><u></u></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<div>
<p class="MsoNormal">Thanks - I was aware of the better MIPS implementation, but I<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">am very hesitant to implement it because I don't have any MIPS<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">hardware with me to test it.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Perhaps someone from mips / imgtec could contribute it once the<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">necessary LLVM infrastructure is in place ? I'm happy to help in any<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">limited way I can.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">I'll upload a new patch shortly with a better ARM implementation (and<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">to address jfb's comments)<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"> <u></u><u></u></p>
<div>
<p class="MsoNormal">On Fri, Mar 7, 2014 at 2:03 PM, Daniel Sanders <<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Sorry for the delay in reviewing this. I had to learn the details before I could comment.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">The ICACHE argument should be BCACHE so that any stale instruction data in the dcache is also flushed. Other than
that the MIPS part of the patch LGTM.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">There is a better implementation available on MIPS32r2 and MIPS64r2 that you could implement if you want to. However
the necessary 'synci' and 'jr.hb' instructions aren't implemented in LLVM yet so you may prefer to leave it for now. The better implementation, uses a series of synci instructions to flush the cache, then a 'sync' followed by a jr.hb to create the necessary
barriers.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Here is an untested version of the better implementation based on the assembly that gcc emits for __builtin_clear_cache():</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">void __clear_cache(void* start, void* end)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">{</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New"">const uintptr_t start_int = (uintptr_t) start;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New"">const uintptr_t end_int = (uintptr_t) end;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New"">if (begin == end)</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New""> return;</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New"">// Discover the step needed for the synci instructions</span><u></u><u></u></p>
<p class="MsoNormal" style="text-indent:36.0pt">
<span style="font-size:11.0pt;font-family:"Courier New"">uintptr_t inc;</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> asm volatile ("rdhwr %0, $1" : "=r"(inc));</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> if (inc == 0)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> return;</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // Round the start address downwards</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> start_int &= ~inc;</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // Flush the region</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> while (start_int < end_int) {</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> asm volatile ("synci %0", :: "r"(start_int) : "memory");</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> start_int += inc;</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> }</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // Memory barrier</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> asm volatile ("sync" ::: "memory");</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // Execution hazard and instruction hazard barrier</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // The jr.hb is the important instruction and could be used in place</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // of the jr instruction that returns from this function rather than</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> // faking a return like this.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> asm volatile ("bal 1f\n\t"</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> "nop\n\t"</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> "1: addiu $31, $31, 12\n\t"</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> "jr.hb $31\n\t"</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New""> "nop\n\t" ::: "$31", "memory");</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Courier New"">}</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><u></u><u></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>]
<b>On Behalf Of </b>Narayan Kamath<br>
<b>Sent:</b> 26 February 2014 16:36<br>
<b>To:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<b>Cc:</b> Jean-Luc Brouillet<br>
<b>Subject:</b> [patch] implement __clear_cache for arm32 & mips</span><u></u><u></u></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<div>
<p class="MsoNormal">Note that both arm32 and mips flavours of this code generate<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">a syscall. <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<p class="MsoNormal">_________________<br>
Save Rainforests <u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<p class="MsoNormal"><br>
<br clear="all">
<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<p class="MsoNormal">--
<br>
_________________<br>
Save Rainforests <u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<p class="MsoNormal"><br>
<br clear="all">
<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<p class="MsoNormal">--
<br>
_________________<br>
Save Rainforests <u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<p class="MsoNormal"><br>
<br clear="all">
<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<p class="MsoNormal">-- <br>
_________________<br>
Save Rainforests <u></u><u></u></p>
</div>
</div></div></div>
</div>
</div>
</blockquote></div><br></div>