LEON Sparc Sub-targets upgrade - first commit

[James Y Knight via llvm-commits]

On Aug 27, 2015, at 2:58 AM, Chris.Dewhurst via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> The attached patch is an initial commit as part of a paid project I’m working on for the European Space Agency (ESA / ESTEC) to add LEON processor support to LLVM.
>  
> LEON processors are fault tolerant processors based on the Sparc V8 processor line. The clearest way to implement this will be to implement it as a sub-target of the Sparc target. See the following for some details in overview: https://en.wikipedia.org/wiki/LEON
>  
> I have a large part of this code working, but as this is my first submittal to LLVM commits, I want to keep it small and hopefully uncontentious as the main purpose of this task is to familiarise myself with the LLVM committal process and develop my own list of incremental changes that will add to the LLVM system.
>  
> Please note that this is an ongoing project for me and the project is a full-time, paid position for which I will be able to continue to dedicate time. The project has a large, multinational sponsor in the European Space Agency. However, as a newcomer to open-source projects, please be gentle with me as I take this, my first, tentative step towards being an active developer on LLVM. I’m bound to make a mistake or two initially.

Welcome, it's always nice to see more people interested in Sparc! :)

I'd appreciate if you could please post your reviews to phabricator instead of just emailing them to the list, as it makes doing the reviews easier. See http://llvm.org/docs/Phabricator.html for more info on how to do this.

I'd like to just highlight one part of the instructions there which trips up almost everyone -- make extra SURE you add a mailing list as a subscriber to each review before sending it out. And if you fail to do so, it's best to just "abandon" the review and create a new one. As the docs say (but perhaps not prominently enough): "If your patch is for LLVM, add llvm-commits as a Subscriber; if your patch is for Clang, add cfe-commits."

I'd also recommend setting up the command-line "Arcanist" tool as mentioned in those docs, even though it's weirdly a PHP commandline application, as running "arc diff" is a *much* easier way to upload diffs than trying to do it via the webui.

Finally, as to your actual patch.

It's really a bit hard to review that in isolation without seeing more.

I'd have expected the first step here to be to add the new processor types (E.g. def: Proc<"leon4", ...>), not to add unused feature tags -- the features are basically internal, and aren't particularly useful to have until there's actually something that needs to depend on it.

I can guess that future patches might add some instruction defintions that use these features? But it's a lot nicer to review changes without having to *guess* what might come in the future.

Thanks!

James