On 10 September 2013 22:15, Jim Grosbach <[email protected]> wrote: > 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. >
Hi Jim, Bob, Thanks for the review! 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. 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. 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. One case, which I fixed by introducing the changed triple to default CPU detection, is the change from "armv6m" to "thumbv6m". 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... Now, on to specific comments... > > On Sep 10, 2013, at 1:58 PM, Bob Wilson <[email protected]> wrote: > > > 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”. > > > +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? > Not important right now, I'll drop it. 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. > > 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. > 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. Again, not important for now. 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. > > I'll use "v7" for everything, for now. So, in the end, there are three changes: 1. Adding all Cortex-A CPUs to CPU-related functions 2. Fix NEON+VFP3 for some Cortex-A CPUs 3. Fix v6M CPU detection I'll split in three patches and re-send. Thanks! --renato
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
