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

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 5 January 2016 at 17:24, William Dillon <william at housedillon.com>
wrote:
> How is this for a proposed solution?  What if I add logic to
> llvm::ARM::getCanonicalArchName() 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.
This is the final "better" solution, but it can't be implemented
right
now due to current expectations of what "armv7l" and
"armv6l" mean.

If you return "v7" from "armv7l", it'll set the arch to
generic ARMv7
which has none of the current ARMv7A attributes that makes code runs
*much* faster. That would be a major regression in performance.

The quick hack is to leave "armv7l" as it is, add "armv6l"
to the
expected "armv6?" variant that means RaspberryPi, and add a number of
FIXMEs to the parser:
 1. To remove both L/B variants as aliases
 2. To implement L/B in getCanonical in the same way as EB/BE
 3. But only once Triple/Tuple can replace the "v7l" -> "v7a
when
Linux" OR "v7R when RTLinux" logic (I have no idea how this will
pan
out).

To know about the progress on Tuple, check with Daniel Sanders and
Eric Christopher (CC'd).

Adding this hack to the Driver now could make the mess even worse at this stage.

cheers,
--renato

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 --------------
A non-text attachment was scrubbed...
Name: llvm-armv6l.diff
Type: application/octet-stream
Size: 2198 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160106/31b7528c/attachment.obj>
-------------- next part --------------
> On Jan 5, 2016, at 9:51 AM, Renato Golin <renato.golin at linaro.org>
wrote:
> 
> On 5 January 2016 at 17:24, William Dillon <william at
housedillon.com> wrote:
>> How is this for a proposed solution?  What if I add logic to
>> llvm::ARM::getCanonicalArchName() 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.
> 
> This is the final "better" solution, but it can't be
implemented right
> now due to current expectations of what "armv7l" and
"armv6l" mean.
> 
> If you return "v7" from "armv7l", it'll set the
arch to generic ARMv7
> which has none of the current ARMv7A attributes that makes code runs
> *much* faster. That would be a major regression in performance.
> 
> The quick hack is to leave "armv7l" as it is, add
"armv6l" to the
> expected "armv6?" variant that means RaspberryPi, and add a
number of
> FIXMEs to the parser:
> 1. To remove both L/B variants as aliases
> 2. To implement L/B in getCanonical in the same way as EB/BE
> 3. But only once Triple/Tuple can replace the "v7l" ->
"v7a when
> Linux" OR "v7R when RTLinux" logic (I have no idea how this
will pan
> out).
> 
> To know about the progress on Tuple, check with Daniel Sanders and
> Eric Christopher (CC'd).
> 
> Adding this hack to the Driver now could make the mess even worse at this
stage.
> 
> cheers,
> --renato

Sorry for the slow reply. I returned from holiday on Friday and was immediately
given urgent bugs to fix.
> To know about the progress on Tuple, check with Daniel Sanders and
> Eric Christopher (CC'd).
> 
TargetTuple has more or less died out but MCTargetMachine is intended to deal
with the majority of the problem in a different way. The latest on the
MCTargetMachine patches is that Eric wanted TargetMachine and MCTargetMachine to
have an is-a relationship but I found this introduced the diamond problem
(because both have target-specific subclasses) and wanted to resolve it using
has-a. Eric said he'd look into it again and get back to me. Meanwhile,
I'm intending to write some temporary patches to enable us to fix some of
the backend issues that MCTargetMachine was going to resolve. These temporary
patches won't help with the triple problems and they'll require making
several create*() functions take MCTargetOptions (which isn't necessary for
most targets) but it will help me make some progress on the various ABI issues
affecting MIPS.