This is an archive of the discontinued LLVM Phabricator instance.

[Nios2] Target registration
ClosedPublic

Authored by AndreiGrischenko on Apr 29 2017, 6:22 AM.

Details

Summary

Hi,

Previously I sent an RFC regarding Nios2 backend introduction in LLVM compiler. It was 12 of April with subject "[llvm-dev] [RFC] Nios II backend".

So there is the first patch for registering Nios2 target.
I registered triple for Nios2, created Nios2 directory in Target and added several initial files for Nios2 target.
In this patch I introduced general classes for Nios2 instructions and register set. An example of one instruction is also added.
So my aim was to prepare patch without tons of lines of code to be simple for review. Let me know, please, if anyway this patch is too big. Or probably, in opposite, you would like to have more changes related to Nios2 target.
I checked llc with this patch, it accepts Nios2 triple and architecture. In the next patch I plan to introduce Nios2 subtargets and cover R1 instructions. (Nios2 architecture has two ISA, R1 and R2. R1 is described here: https://www.altera.com/en_US/pdfs/literature/hb/nios2/n2cpu-nii5v1gen2.pdf)

Current status of Nios2 project is I can produce correct assembly for simple test cases. That assembly is checked for correctness on Nios2 hardware.

To test that I did not break anything, I built common targets and ran "make check".

I'm open for any questions, suggestions, advices and objections.

Thanks in advance,
Andrei.

Diff Detail

Event Timeline

igorb added a subscriber: igorb.May 16 2017, 8:55 AM
DavidKreitzer added inline comments.May 17 2017, 6:15 AM
../llvm/lib/Target/Nios2/CMakeLists.txt
1

Is there a clear LLVM policy regarding these svn attributes? The repository is pretty inconsistent, but you should at least be consistent within your patch. I suspect that you want to remove them.

craig.topper added inline comments.May 17 2017, 1:02 PM
../llvm/CMakeLists.txt
315

Do we want this as part of the default build yet? Or should it be experimental and optin for now? WebAssembly for example isn't listed here and must be explicitly built by passing it to cmake.

../llvm/lib/Target/Nios2/LLVMBuild.txt
49

I think these should be in alphabetical order. That's consistent with what other targets did before GlobalISel started getting added to the end of everyone's list.

../llvm/lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.h
1

Don't line wrap this first line comment. -*- C++ -*- is a magic token for text editors and needs to be together. This applies to several files in this review.

../llvm/lib/Target/Nios2/Nios2TargetMachine.cpp
28

Can we just return the string without the temporary variable?

../llvm/CMakeLists.txt
315

Yes, I assume at the moment we do not want that. I will remove Nios2 from there.

../llvm/lib/Target/Nios2/CMakeLists.txt
1

I do not know about that policy. And yes I will remove these attributes.

../llvm/lib/Target/Nios2/LLVMBuild.txt
49

OK

../llvm/lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.h
1

OK, I will fix that

../llvm/lib/Target/Nios2/Nios2TargetMachine.cpp
28

Sure. Thanks.

I've updated patch according to comments from David and Craig.

craig.topper added inline comments.May 24 2017, 1:55 PM
llvm/lib/Target/Nios2/LLVMBuild.txt
1 ↗(On Diff #99563)

80 columns. I think the first line of many of your files are too long.

llvm/lib/Target/Nios2/Nios2InstrFormats.td
40 ↗(On Diff #99563)

Are the OpxH and shamt fields used?

97 ↗(On Diff #99563)

Is this unused?

Is there enough functional here that there should be tests for? i.e. make sure march/mcpu switches are recognized, that the target is recognized, etc.

Initially I thought about adding such check for the triple is supported. But it was not obvious how to do that and llvm-experienced guys around said it was not a big sense to add tests at this stage.
Even now I use "not not llc" to make my lit test work. Anyway I tried to create such test case, please look. The test checks for -mtriple=nios2 and -march=nios2 are accepted. Further I will expand this tests with -mcpu after adding CPUs.

llvm/lib/Target/Nios2/LLVMBuild.txt
1 ↗(On Diff #99563)

I cannot explain why that happened. Will fix.

llvm/lib/Target/Nios2/Nios2InstrFormats.td
40 ↗(On Diff #99563)

No at the moment. I will remove them.

97 ↗(On Diff #99563)

Yes, this is unused. I will update this instruction format to be closer to our current version.
There are just initial formats are added. Nios2 has more instruction formats and I plan to expand that further.

Changes according to Craig's comments.

This revision is now accepted and ready to land.May 25 2017, 10:24 AM
This revision was automatically updated to reflect the committed changes.