llvm dev - Jan 2016 - Diff to add ARMv6L to Target parser

> You assume triples make sense. That's the first mistake everyone does
> when thinking about triples. :)
I know they don't make sense in many corner cases, but I think
discarding logic where it *does* exist is a mistake.
> AFAIK, "ARMv7B" is only used by HighBank, which is no more. But
that,
> too, was "ARMv7A big endian".
I believe it's what any Linux kernel will present itself as via "uname
-a" which, as James said, is a vaguely sane way to seed a triple.
> The ARM documents define a list of official architecture names, and
> those are the ones supported by the TargetParser. What the
> Triple/Tuple supports is completely orthogonal.
I do know this, Renato.
> So, in the TargetParser, there can be no "ARMv7L" nor
"ARMv7B", only
> "ARMv7A". The other strings map to ARMv7A because that's what
they
> generally mean, in the wild, with regards to "architecture
names". The
> hack is that the Driver/Triple/Tuple are using that to mean everything
> else.
Sure. In exactly the same way there's no "armebv7" in
TargetParser.
There's principled(ish) parsing of the "eb" when deciding what the
triple actually means though.
>> What does RTLinux on an R-class CPU present itself as? My guess would
>> be "armv7l" too.
>
> Nope. GNU triples have a very specific meaning because that's
what's
> generated from the configure-time options.
I have no idea what you're objecting to here. I wasn't saying anything
about how GNU interprets the armv7l that would be produced, just that
I would expect running "uname" on an R-class Linux to produce armv7l.

Tim.

> I believe it's what any Linux kernel will present itself as via
"uname
> -a" which, as James said, is a vaguely sane way to seed a triple.
Well, not necessarily sane, but almost certainly something people will
do anyway.

Tim.

On 5 January 2016 at 12:33, Tim Northover <t.p.northover at gmail.com>
wrote:
> I know they don't make sense in many corner cases, but I think
> discarding logic where it *does* exist is a mistake.
We're not discarding any logic. As I said, the architecture name
cannot be ARMv7L for any reason. It's that simple.

The logic, if it exists, need to be encoded somewhere else.

> I do know this, Renato.
I only meant as an opener, not as education. I know you know this...

> I have no idea what you're objecting to here. I wasn't saying
anything
> about how GNU interprets the armv7l that would be produced, just that
> I would expect running "uname" on an R-class Linux to produce
armv7l.
I cannot separate between A and R if both use the same arch name.
Knowing the sub-arch is as important than knowing its byte-sex, and I
can't trade one for the other.

If the Driver/Triple/Tuple knows which, depending on other
information, then they should be the ones encoding this.

Adding an *Arch Name* ARMv7L will make no progress in understanding
what it is. Encoding the correct fields in the Triple/Tuple will, and
for that, ARMv7A or ARMv7R are the *correct* "arch names".

cheers,
--renato

Hi all,

Thank you so much for this discussion.  It has been very helpful and
educational.  I think that I understand the perspectives of both Tim and Renato.
Let me briefly summarize them to ensure that I correctly understand:

Renato’s perspective is that the triple means whatever the author says it does,
and that may or not be meaningful.  :) In the case of armv7l (for example) it
has a clear meaning: ARMv7A little endian.  Unfortunately, this nomenclature
collides with what is used for sub-architectures.  ARMv7l, however, is NOT a
sub-architecture, and should not be treated as one.

Tim’s perspective is that what Renato says isn’t wrong, but that while the logic
currently used to map armv7l into something meaningful is not great, it is
certainly not unacceptable.  Given that armv7l is already treated in this way,
it is not unreasonable to also support armv6l.

How is this for a proposed solution?  What if I add logic to
llvm::ARM::getCanonicalArchName()
<https://github.com/apple/swift-llvm/blob/stable/lib/Support/TargetParser.cpp#L387>
that matches the trailing ‘l’ (or ‘b’) and treats it the same way that ‘el’ and
‘eb’ are now?  This function would return v7 or v6 in the case of armv7l or
armv6l, respectively (or, if it’s preferred, v7a or v6a).  It seems like this is
closer to the “correct” approach.  It does show potentially inappropriate
deference to Linux’s choice in the matter, but at least it’s not making
sub-architectures that don’t exist.

Thoughts?
- Will
> On Jan 5, 2016, at 4:48 AM, Renato Golin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On 5 January 2016 at 12:33, Tim Northover <t.p.northover at
gmail.com> wrote:
>> I know they don't make sense in many corner cases, but I think
>> discarding logic where it *does* exist is a mistake.
> 
> We're not discarding any logic. As I said, the architecture name
> cannot be ARMv7L for any reason. It's that simple.
> 
> The logic, if it exists, need to be encoded somewhere else.
> 
> 
>> I do know this, Renato.
> 
> I only meant as an opener, not as education. I know you know this...
> 
> 
>> I have no idea what you're objecting to here. I wasn't saying
anything
>> about how GNU interprets the armv7l that would be produced, just that
>> I would expect running "uname" on an R-class Linux to produce
armv7l.
> 
> I cannot separate between A and R if both use the same arch name.
> Knowing the sub-arch is as important than knowing its byte-sex, and I
> can't trade one for the other.
> 
> If the Driver/Triple/Tuple knows which, depending on other
> information, then they should be the ones encoding this.
> 
> Adding an *Arch Name* ARMv7L will make no progress in understanding
> what it is. Encoding the correct fields in the Triple/Tuple will, and
> for that, ARMv7A or ARMv7R are the *correct* "arch names".
> 
> cheers,
> --renato
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160105/bea1f650/attachment.html>

> On Jan 7, 2016, at 2:17 PM, William Dillon via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Yikes!!
> 
> It looks like things have changed considerably!  I’ll need to start this
from scratch.  A few questions, though:
> 
> I have a goal for this to be in the Swift 2.2 release, is that feasible?
> If so, will the current LLVM head end up in the branch for 2.2 when that
time comes?
Hi Will,

The swift-dev list (swift-dev at swift.org <mailto:swift-dev at
swift.org>) is the right place for those questions. Ted Kremenek sent a
message regarding Swift 2.2 earlier in the week:
https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20160104/000690.html
<https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20160104/000690.html>

You might consider reading the blog post he refers to and replying to his
message if you’re looking for any clarification.

Mark


> 
> Given that the coordination costs to attempt any kind of change in swift
that requires a change in LLVM are so high, I’m tempted to keep the logic of
stripping the ‘l’s from armv7l and armv6l inside swift itself.  It really seems
like the wrong approach, but sometimes the wrong answer is the best answer,
depending on the circumstances.
> 
> - Will
> 
>> On Jan 7, 2016, at 10:05 AM, Eric Christopher <echristo at gmail.com
<mailto:echristo at gmail.com>> wrote:
>> 
>> The swift repository is old and many thousand revisions behind llvm.
Please don't use it as a base when submitting to the llvm project.
>> 
>> 
>> On Thu, Jan 7, 2016, 10:02 AM William Dillon <william at
housedillon.com <mailto:william at housedillon.com>> wrote:
>> Oops, I neglected to reply-all….
>> 
>> The current stable branch at github still has it:
>> 
>>
https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106
<https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106>
>> 
>> Should I get the head of the non-swift repository and generate a new
diff?
>> 
>> Also, I suspect that it’s not a good idea to have armv6l map to armv6k,
because that seems like quite an assumption to make.  Clearly, armv6 sub
architectures that aren’t v6k will still be v6l in linux. (provided they’re
little-endian).
>> I’ve already made that change, and it would be included in any revised
diff that I send out.
>> 
>> Thanks,
>> - Will
>> 
>>> On Jan 6, 2016, at 10:02 AM, Artyom Skrobov <Artyom.Skrobov at
arm.com <mailto:Artyom.Skrobov at arm.com>> wrote:
>>> 
>>> William, what revision of LLVM is your patch based on?
>>> 
>>> The trunk hasn't had ARM_ARCH("armv6hl") since
r252903 (Nov 12th)
>>> 
>>> 
>>> -----Original Message-----
>>> From: William Dillon [mailto:william at housedillon.com
<mailto:william at housedillon.com>]
>>> Sent: 06 January 2016 17:55
>>> To: Renato Golin
>>> Cc: Tim Northover; LLVM Dev; nd; Artyom Skrobov; Daniel Sanders;
Eric Christopher
>>> Subject: Re: [llvm-dev] Diff to add ARMv6L to Target parser
>>> 
>>> Taking the suggestions of the group under consideration, I’ve
generated a new diff.  The thing to note is that armv6l is now treated
identically to armv6hl.  I’ve also added a unit test.
>>> This seems to me to be the least invasive method, and holds to
existing conventions as closely as possible.
>>> 
>>> Thoughts?
>>> 
>> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160107/41b19939/attachment.html>

> On Jan 7, 2016, at 2:21 PM, Eric Christopher <echristo at gmail.com>
wrote:
> 
> 
> 
> On Thu, Jan 7, 2016 at 2:17 PM William Dillon <william at
housedillon.com <mailto:william at housedillon.com>> wrote:
> Yikes!!
> 
> It looks like things have changed considerably!  I’ll need to start this
from scratch.  A few questions, though:
> 
> I have a goal for this to be in the Swift 2.2 release, is that feasible?
Maybe. We’re using an older version of llvm/clang for Swift 2.2 to give us a
stable platform to work with, but if you get a change into trunk first, and if
it is relatively low-risk, we could consider back-porting it to the release
branch for Swift 2.2. We do want to be careful to avoid destabilizing the
branch, though, and the criteria for accepting changes will only get more strict
over time.
> If so, will the current LLVM head end up in the branch for 2.2 when that
time comes?
No. We already created the llvm/clang release branches to be used with Swift 2.2
to give us a longer time to stabilize them. We do not plan to rebranch again
from trunk for Swift 2.2.
> 
> Given that the coordination costs to attempt any kind of change in swift
that requires a change in LLVM are so high, I’m tempted to keep the logic of
stripping the ‘l’s from armv7l and armv6l inside swift itself.  It really seems
like the wrong approach, but sometimes the wrong answer is the best answer,
depending on the circumstances.
> 
> 
> For all of these questions you should talk to Bob Wilson who is the llvm
release manager for the swift project.
> 
> -eric
>  
> - Will
> 
>> On Jan 7, 2016, at 10:05 AM, Eric Christopher <echristo at gmail.com
<mailto:echristo at gmail.com>> wrote:
>> 
>> The swift repository is old and many thousand revisions behind llvm.
Please don't use it as a base when submitting to the llvm project.
>> 
>> 
>> On Thu, Jan 7, 2016, 10:02 AM William Dillon <william at
housedillon.com <mailto:william at housedillon.com>> wrote:
>> Oops, I neglected to reply-all….
>> 
>> The current stable branch at github still has it:
>> 
>>
https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106
<https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106>
>> 
>> Should I get the head of the non-swift repository and generate a new
diff?
>> 
>> Also, I suspect that it’s not a good idea to have armv6l map to armv6k,
because that seems like quite an assumption to make.  Clearly, armv6 sub
architectures that aren’t v6k will still be v6l in linux. (provided they’re
little-endian).
>> I’ve already made that change, and it would be included in any revised
diff that I send out.
>> 
>> Thanks,
>> - Will
>> 
>>> On Jan 6, 2016, at 10:02 AM, Artyom Skrobov <Artyom.Skrobov at
arm.com <mailto:Artyom.Skrobov at arm.com>> wrote:
>>> 
>>> William, what revision of LLVM is your patch based on?
>>> 
>>> The trunk hasn't had ARM_ARCH("armv6hl") since
r252903 (Nov 12th)
>>> 
>>> 
>>> -----Original Message-----
>>> From: William Dillon [mailto:william at housedillon.com
<mailto:william at housedillon.com>]
>>> Sent: 06 January 2016 17:55
>>> To: Renato Golin
>>> Cc: Tim Northover; LLVM Dev; nd; Artyom Skrobov; Daniel Sanders;
Eric Christopher
>>> Subject: Re: [llvm-dev] Diff to add ARMv6L to Target parser
>>> 
>>> Taking the suggestions of the group under consideration, I’ve
generated a new diff.  The thing to note is that armv6l is now treated
identically to armv6hl.  I’ve also added a unit test.
>>> This seems to me to be the least invasive method, and holds to
existing conventions as closely as possible.
>>> 
>>> Thoughts?
>>> 
>> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160108/c163711c/attachment-0001.html>

Thanks for the clarifications, Bob!

I’ve spent some time with the head of the llvm.org repo, and I now understand a
lot better what Renato and Tim were talking about re. the architecture aliases. 
The patch to add v6l, therefore, seems simple enough.  I haven’t been able to
test it in my usual flow, because that involves the whole swift stack.  I’m
considering creating a program that links to llvm to test the behavior.

Index: lib/Support/TargetParser.cpp
==================================================================---
lib/Support/TargetParser.cpp	(revision 257090)
+++ lib/Support/TargetParser.cpp	(working copy)
@@ -401,6 +401,7 @@
       .Case("v5", "v5t")
       .Case("v5e", "v5te")
       .Case("v6j", "v6")
+      .Case("v6l", "v6")
       .Case("v6hl", "v6k")
       .Cases("v6m", "v6sm", "v6s-m",
"v6-m")
       .Cases("v6z", "v6zk", "v6kz")
Index: unittests/ADT/TripleTest.cpp
==================================================================---
unittests/ADT/TripleTest.cpp	(revision 257090)
+++ unittests/ADT/TripleTest.cpp	(working copy)
@@ -851,6 +851,10 @@
     EXPECT_EQ("arm1136jf-s", Triple.getARMCPUForArch());
   }
   {
+    llvm::Triple Triple("armv6l-unknown-eabi");
+    EXPECT_EQ("arm1136jf-s", Triple.getARMCPUForArch());
+  }
+  {
     llvm::Triple Triple("armv6j-unknown-eabi");
     EXPECT_EQ("arm1136jf-s", Triple.getARMCPUForArch());
   }


I looked into the tests (and unit tests), and found that perhaps the most
appropriate test to add is in the determination of the cpu type from the
architecture version.  I verified that ARM1136 is the base for ARM11, which is
all that can be assumed given armv6.  That jives with the existing armv6
architecture test.  I added another copy of that for armv6l.  I assume that this
will sufficiently test whether armv6l is aliasing correctly to armv6.

Anyway, thanks again.
- Will
> On Jan 8, 2016, at 10:16 AM, Bob Wilson <bob.wilson at apple.com>
wrote:
> 
> 
>> On Jan 7, 2016, at 2:21 PM, Eric Christopher <echristo at gmail.com
<mailto:echristo at gmail.com>> wrote:
>> 
>> 
>> 
>> On Thu, Jan 7, 2016 at 2:17 PM William Dillon <william at
housedillon.com <mailto:william at housedillon.com>> wrote:
>> Yikes!!
>> 
>> It looks like things have changed considerably!  I’ll need to start
this from scratch.  A few questions, though:
>> 
>> I have a goal for this to be in the Swift 2.2 release, is that
feasible?
> 
> Maybe. We’re using an older version of llvm/clang for Swift 2.2 to give us
a stable platform to work with, but if you get a change into trunk first, and if
it is relatively low-risk, we could consider back-porting it to the release
branch for Swift 2.2. We do want to be careful to avoid destabilizing the
branch, though, and the criteria for accepting changes will only get more strict
over time.
> 
>> If so, will the current LLVM head end up in the branch for 2.2 when
that time comes?
> 
> No. We already created the llvm/clang release branches to be used with
Swift 2.2 to give us a longer time to stabilize them. We do not plan to rebranch
again from trunk for Swift 2.2.
> 
>> 
>> Given that the coordination costs to attempt any kind of change in
swift that requires a change in LLVM are so high, I’m tempted to keep the logic
of stripping the ‘l’s from armv7l and armv6l inside swift itself.  It really
seems like the wrong approach, but sometimes the wrong answer is the best
answer, depending on the circumstances.
>> 
>> 
>> For all of these questions you should talk to Bob Wilson who is the
llvm release manager for the swift project.
>> 
>> -eric
>>  
>> - Will
>> 
>>> On Jan 7, 2016, at 10:05 AM, Eric Christopher <echristo at
gmail.com <mailto:echristo at gmail.com>> wrote:
>>> 
>>> The swift repository is old and many thousand revisions behind
llvm. Please don't use it as a base when submitting to the llvm project.
>>> 
>>> 
>>> On Thu, Jan 7, 2016, 10:02 AM William Dillon <william at
housedillon.com <mailto:william at housedillon.com>> wrote:
>>> Oops, I neglected to reply-all….
>>> 
>>> The current stable branch at github still has it:
>>> 
>>>
https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106
<https://github.com/apple/swift-llvm/blob/stable/include/llvm/Support/ARMTargetParser.def#L106>
>>> 
>>> Should I get the head of the non-swift repository and generate a
new diff?
>>> 
>>> Also, I suspect that it’s not a good idea to have armv6l map to
armv6k, because that seems like quite an assumption to make.  Clearly, armv6 sub
architectures that aren’t v6k will still be v6l in linux. (provided they’re
little-endian).
>>> I’ve already made that change, and it would be included in any
revised diff that I send out.
>>> 
>>> Thanks,
>>> - Will
>>> 
>>>> On Jan 6, 2016, at 10:02 AM, Artyom Skrobov <Artyom.Skrobov
at arm.com <mailto:Artyom.Skrobov at arm.com>> wrote:
>>>> 
>>>> William, what revision of LLVM is your patch based on?
>>>> 
>>>> The trunk hasn't had ARM_ARCH("armv6hl") since
r252903 (Nov 12th)
>>>> 
>>>> 
>>>> -----Original Message-----
>>>> From: William Dillon [mailto:william at housedillon.com
<mailto:william at housedillon.com>]
>>>> Sent: 06 January 2016 17:55
>>>> To: Renato Golin
>>>> Cc: Tim Northover; LLVM Dev; nd; Artyom Skrobov; Daniel
Sanders; Eric Christopher
>>>> Subject: Re: [llvm-dev] Diff to add ARMv6L to Target parser
>>>> 
>>>> Taking the suggestions of the group under consideration, I’ve
generated a new diff.  The thing to note is that armv6l is now treated
identically to armv6hl.  I’ve also added a unit test.
>>>> This seems to me to be the least invasive method, and holds to
existing conventions as closely as possible.
>>>> 
>>>> Thoughts?
>>>> 
>>> 
>> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160108/973da779/attachment.html>