bytecodealliance / target-lexicon

Target "triple" support
Apache License 2.0
47 stars 42 forks source link

Add Clever architecture #85

Closed chorman0773 closed 1 year ago

chorman0773 commented 1 year ago

This adds support for the clever architecture, including version specifiers.

Clever-ISA is defined by https://github.com/Clever-ISA/Clever-ISA and the format of targets is defined by the toolchain recommendations technical document (specs/toolchain.md or referred to as D-toolchain). This PR supports both the "bare" clever name specifier, as well as ones with a version suffix. The targets are also supported by the similar target parsing library, target-tuples.

The support is being added so I can add the architecture to a fork cranelift code generator, for the purposes of using it via rustc_codegen_cranelift. Proposing to upstream the modification to cranelift is a possibility depending on how well it goes.

chorman0773 commented 1 year ago

I had one for a standard suggested target from the ISA, but apparently it will add unknown to the os field for clever-unknown-elf.

On Sat, 7 Jan 2023 at 15:49, Dan Gohman @.***> wrote:

@.**** commented on this pull request.

Looks good. Could you also add a full triple round trip test here? https://github.com/bytecodealliance/target-lexicon/blob/main/src/targets.rs#L1338

— Reply to this email directly, view it on GitHub https://github.com/bytecodealliance/target-lexicon/pull/85#pullrequestreview-1239704964, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGLD22AAF623AQYFAXFUHTWRHJGJANCNFSM6AAAAAATTPXVQY . You are receiving this because you authored the thread.Message ID: @.***>

sunfishcode commented 1 year ago

In that case, it sounds like Clever will need special-case logic. It's possible that adding some Clever checks to this check could be sufficient.

chorman0773 commented 1 year ago

That special case seems to avoid adding the wrong unknown to the output :sweat_smile:. clever-unknown-elf (the one actual target defined by D-toolchain, D-toolchain only otherwise defines the arch component and recommends the vendor component) should be parsed as -- form. The output is avoiding printing the unknown vendor, but that component is technically still required, and its the OS that should be omitted (there is, in fact, no os rather than an unknown os - this is the baremetal "elf" only target). While the logical result is the same, and clever-unknown-elf will pass the round-trip, another, argualy correct (D-toolchain states that any vendor is permitted, recommending unknown by default), target such as clever-pc-elf still will not.

This seems to be in the way that target-lexicon handles the sys component - I do have to agree that it is painful to do correctly. If it's possible for a more general solution for omitting the os component, that might be a good idea. Otherwise, I can add a target that passes, but is less "correct" (in terms of the arch), and then workaround the different parsing logic when handling the target name.

bjorn3 commented 1 year ago

For bare metal targets the OS is usually set to "none". For example x86_64-unknown-none.

chorman0773 commented 1 year ago

For bare metal targets the OS is usually set to "none". For example x86_64-unknown-none.

I would currently prefer to use the standardized triple, if for no other reason than because the tooling I'm presently using is already referring to the target by this name. I'm also trying to keep everything nice and standardized between everything, that was the point of D-toolchain. The none OS has no prior art outside of rustc (and the particular part of D-toolchain was written before x86_64-unknown-none).

In any case the none target should suffice as a test triple.

sunfishcode commented 1 year ago

The purpose of the round-trip test is to test that triples used in practice can be round-tripped, so I'd prefer to add the actual triple that's used in practice.

I don't know anything about Clever or D-toolchain. Target-lexicon has not previously encountered any target that omits the OS field. But it seems straightforward to add a new rule to the printing logic: if the architecture is Clever, just print the vendor, and not the operating_system.

chorman0773 commented 1 year ago

You can have an actual OS field. clever-unknown-v1os (used for a testing/PoC OS) should be preserved as-is, so the ad-hoc rule would if architecture is Clever, omit the operating_system iff it is Unknown. I can attempt to implement this later today. The thing that's happening here is that a (canonical) target triple is

--, not --[-]. can either be a standalone component, or two components, typically -. In the GNU config.sub logic, elf is both a valid environment, and a valid standalone system. The spec in play here is relying on that interpretation, but target-lexicon isn't set up to handle it. Whether or not this is intended on the part of target-lexicon (or initially an omission but not something to be fixed), or an accidental omission brought on by lack of example, is not something I'm going to argue on this issue. A proper rule would be similar to what target-tuples does, which is store the 2 (or internally 3, splitting the last component into env/objformat) parts of the sys component as an Option, and only print the ones that are present. On Wed, 11 Jan 2023 at 08:31, Dan Gohman ***@***.***> wrote: > The purpose of the round-trip test is to test that triples used in > practice can be round-tripped, so I'd prefer to add the actual triple > that's used in practice. > > I don't know anything about Clever or D-toolchain. Target-lexicon has not > previously encountered any target that omits the OS field. But it seems > straightforward to add a new rule to the printing logic: if the > architecture is Clever, just print the vendor, and not the > operating_system. > > — > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >
sunfishcode commented 1 year ago

target-lexicon is more similar to LLVM's Triple code than GNU config.sub. In LLVM's Triple, elf is neither an environment or an operating system. It's considered to be a binary format, which is considered a fifth field of the tuple.

chorman0773 commented 1 year ago

I've added the special case, and am now using the proper target in the roundtrip test.

sunfishcode commented 1 year ago

Thanks!