<div dir="ltr">On 10 September 2013 22:15, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">If nothing else, this strikes me as something that should really be multiple district patches. There’s multiple things going on here and I’m not sure what’s what.</div>
</blockquote><div><br></div><div>Hi Jim, Bob,</div><div><br></div><div>Thanks for the review!</div><div><br></div><div>Yes, there's a lot going on and this is a medley of everything. I'll split into multiple patches, but I wasn't sure what's worth and what's not.</div>
<div><br></div><div>Bob, you are right that this is *mostly* a tidy-up without much thought, so please disregard if there's any change that doesn't make sense.</div><div><br></div><div>About the "v7" to "v7a" change, I was just wondering why it was that way, now I know. This change is not necessary for anything, but the main problem with being "v7" is that when you're converting architecture strings in the driver (which happens many times per invocation), the triple changes, and the default CPU is not the same.</div>
<div><br></div><div>One case, which I fixed by introducing the changed triple to default CPU detection, is the change from "armv6m" to "thumbv6m".</div><div><br></div><div>But I don't have any special case, so we can drop that until it becomes relevant. When it does, we'll have to take into account Darwin triples, and that might not be trivial...</div>
<div><br></div><div>Now, on to specific comments...</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>
<br><div><div><div class="h5"><div>On Sep 10, 2013, at 1:58 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>> wrote:</div><blockquote type="cite"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<br>This is going to change the triples to use "armv7a" instead of "armv7".  What have you done to test that?  How important is it to make that change?  We could just continue to use "armv7" in place of "armv7a”.<br>
</div></blockquote><div><br></div></div></div>+1. I don’t follow why this is a good change to make. If it’s just nomenclature tidy-up, I really don’t think it’s worth it. Is there more too it than that?</div></div></div></blockquote>
<div><br></div><div>Not important right now, I'll drop it.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div><div class="im"><blockquote type="cite" dir="auto"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
What is armv7l for?  I see one place in lib/Driver/ToolChain.cpp where it is mapped to a cortex-a8 processor, but that doesn't make sense to me.<br></div></blockquote></div><div>First I’ve ever heard of it, too. I have no philosophical objection, really, but would like a bit more clarity on what it actually is if that’s possible.</div>
</div></div></div></blockquote><div><br></div><div>According to the GCC folks, it's for "little-endian", and it's used by Linux distributions to compile the kernel. This change is not required by anything now. The default on Cortex-A8 is because that's the oldest v7 that runs Linux kernels at the moment.</div>
<div><br></div><div>Again, not important for now.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div><div><div class="h5"><blockquote type="cite" dir="auto"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
Aside from adding cortex-a12, this shouldn't change anything.  The result of that function is either only compared to the "v7" prefix or handled in the StringSwitch at the end of getARMTargetCPU, where "v7" and "v7a" are treated the same.<br>
</div></blockquote></div></div></div></div></div></blockquote><div>I'll use "v7" for everything, for now.</div><div><br></div><div><br></div><div>So, in the end, there are three changes:</div><div> 1. Adding all Cortex-A CPUs to CPU-related functions</div>
<div> 2. Fix NEON+VFP3 for some Cortex-A CPUs</div><div> 3. Fix v6M CPU detection</div><div><br></div><div>I'll split in three patches and re-send.</div><div><br></div><div>Thanks!</div><div>--renato</div></div></div>
</div>