This is an archive of the discontinued LLVM Phabricator instance.

[Nios2] Arithmetic instructions for R1 and R2 ISA.
ClosedPublic

Authored by belickim on Dec 14 2017, 6:44 AM.

Details

Summary

This commit enables some of the arithmetic instructions for Nios2 ISA (for both R1 and R2 revisions), implements facilities required to emit those instructions and provides LIT tests for added instructions.

Diff Detail

Event Timeline

belickim created this revision.Dec 14 2017, 6:44 AM
lib/Target/Nios2/Nios2InstrInfo.td
78

Do you need this commented out?

test/CodeGen/Nios2/add-sub.ll
2

I think it makes sense to add test for R2 version as well, because you are introducing R2 in this patch. I know that you will not get difference in assembly among both versions, but we need to be sure in that.

belickim updated this revision to Diff 127101.Dec 15 2017, 4:37 AM

Added R2 run to LIT tests, fixed a typo with unintended comment.

belickim marked 2 inline comments as done.Dec 15 2017, 4:38 AM
hfinkel added inline comments.Dec 15 2017, 7:51 PM
lib/Target/Nios2/Nios2ISelLowering.cpp
201

Merge init with the declaration on the previous line.

207

llvm_unreachable takes a string describing the problem.

lib/Target/Nios2/Nios2InstrInfo.cpp
53

Is this function being called without a valid destination or source register?

lib/Target/Nios2/Nios2InstrInfo.td
88

Would you really want to rematerialize division on this architecture? Only declare these as rematerializable if they're cheaper than a spill/restore.

belickim updated this revision to Diff 127502.Dec 19 2017, 5:09 AM

Thanks for the review Hal. When I was addressing the things that you pointed out I found that entire analyzeReturn method is already implemented in LLVM, which allowed me to remove some code.

belickim marked 4 inline comments as done.Dec 19 2017, 5:10 AM
hfinkel added inline comments.Dec 19 2017, 7:38 PM
test/CodeGen/Nios2/add-sub.ll
7

Aren't all of the registers here fixed by the ABI? If so, check for them explicitly instead of matching any register.

belickim updated this revision to Diff 128626.Jan 4 2018, 9:43 AM

Thanks for another comment Hal, I've updated the test accordingly.

belickim marked an inline comment as done.Jan 4 2018, 9:44 AM

Are you planning to soon introduce an asm parser and disassembler so that you can test the encodings? I recommend doing this early do that you can test those along the way.

lib/Target/Nios2/Nios2InstrInfo.cpp
53

We generally write this as:

BuildMI(MBB, I, DL, get(opc), DestReg)
  .addReg(Nios2::ZERO);
  .addReg(SrcReg, getKillRegState(KillSrc));

Are you planning to soon introduce an asm parser and disassembler so that you can test the encodings? I recommend doing this early do that you can test those along the way.

Yes, we plan to support asm parser in disassembler, but I would not say these were nearby plans. Currently we can just generate assembler and then use GCC as with Nios2 support to generate object-files. But I'm OK with your recommendation, I think we can focus on that before continue committing new instructions.

belickim updated this revision to Diff 128897.Jan 8 2018, 1:53 AM

I've fixed the coding style issue in Nios2InstrInfo::copyPhysReg.

belickim marked an inline comment as done.Jan 8 2018, 1:53 AM
hfinkel accepted this revision.Jan 8 2018, 7:43 PM

LGTM

lib/Target/Nios2/Nios2InstrInfo.cpp
50

You can now drop the MachineInstrBuilder MIB = part too.

This revision is now accepted and ready to land.Jan 8 2018, 7:43 PM
belickim updated this revision to Diff 129039.Jan 9 2018, 12:12 AM

Thank you Hal, I removed the unused variable.

belickim marked an inline comment as done.Jan 9 2018, 12:12 AM
This revision was automatically updated to reflect the committed changes.