This is an archive of the discontinued LLVM Phabricator instance.

[Nios2] final infrastructure addition to provide compilation of simple return from a function.
ClosedPublic

Authored by AndreiGrischenko on Nov 6 2017, 8:26 AM.

Details

Summary

Hi,

Please review next patch for Nios2 code-gen.

This huge patch includes all missing functionality needed to provide first compilation of a program that simply returns from a function.
I've added a test case that checks for "ret" instruction printed in assembly output.

Next patches will be smaller. They will bring Nios2 R1, R2 ISA, instruction lowering and etc.

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.Nov 17 2017, 11:44 AM
lib/Target/Nios2/CMakeLists.txt
22 ↗(On Diff #121739)

Frame lowering should be above InstrInfo.cpp to be in alphabetical ordering

lib/Target/Nios2/InstPrinter/Nios2InstPrinter.cpp
29 ↗(On Diff #121739)

Are your register names not lowercase in the td file?

Though it seems several targets do this.

lib/Target/Nios2/MCTargetDesc/CMakeLists.txt
2 ↗(On Diff #121739)

Can you put these on one line per file and alphabetize?

lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
51 ↗(On Diff #121739)

The if at line 49 check if CPU is empty or generic. There's no way to get here and the CPU be "nios2r2"

Updates according to Craig's comments.

lib/Target/Nios2/CMakeLists.txt
22 ↗(On Diff #121739)

OK

lib/Target/Nios2/InstPrinter/Nios2InstPrinter.cpp
29 ↗(On Diff #121739)

Yes, right, thanks. It is not necessary for Nios2 as all registers are in lowercase.

lib/Target/Nios2/MCTargetDesc/CMakeLists.txt
2 ↗(On Diff #121739)

OK

lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
51 ↗(On Diff #121739)

Yes, I reworked this routine.

craig.topper added inline comments.Nov 20 2017, 9:28 AM
lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
49 ↗(On Diff #123608)

That's a comparison not an assignment.

echristo added inline comments.Nov 20 2017, 10:07 AM
lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
49 ↗(On Diff #123608)

Which also means an untested code path - relatedly, there are no tests here.

lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
49 ↗(On Diff #123608)

omg! Very stupid typo.

49 ↗(On Diff #123608)

Not exactly. This patch is aimed to start printing "ret" into assembly output, so I've added appropriate test case for that. Further I plan to add more test cases when adding exact things, not just infrastructure.
However I will add one more test checking that Nios2 processors are accepted.

lib/Target/Nios2/MCTargetDesc/Nios2MCTargetDesc.cpp
49 ↗(On Diff #123608)

Current patch does not provide enough functionality to check that CPU=empty is incorrect.

Fixed a typo when assigning default CPU for Nios2.
Added a test checking for acceptance of Nios2 processors.

craig.topper accepted this revision.Nov 28 2017, 7:10 PM
This revision is now accepted and ready to land.Nov 28 2017, 7:10 PM

Hi Craig,

Thanks for acceptance the patch. Unfortunately a person who makes commits declined the patch because formally only small part of it was accepted.
Now I update diffs again to bring the whole patch again as unified diff(with changes regarding your comments). Could you please accept it again?
Sorry for that.

Thanks,
Andrei.

craig.topper accepted this revision.Dec 5 2017, 8:12 AM

LGTM other than the one comment. Please fix before you commit.

craig.topper added inline comments.Dec 6 2017, 9:48 AM
lib/Target/Nios2/Nios2RegisterInfo.cpp
39 ↗(On Diff #125080)

This should be MCPhysReg not uint16_t.

lib/Target/Nios2/Nios2RegisterInfo.cpp
39 ↗(On Diff #125080)

OK, I will fix it before the commit.

This revision was automatically updated to reflect the committed changes.