Page MenuHomePhabricator

[RISCV 11/n] Initial codegen support for ALU operations
ClosedPublic

Authored by asb on Feb 14 2017, 4:56 AM.

Details

Summary

I think this is a fair approximation of the minimum necessary for some sort of testable codegen - though if people have any ideas on splitting things up further I'm all ears. Frame handling, and proper function prologue/epilogue handling is left for a later patch.

For the time being, this primarily targets RV32. Last I heard, @kparzysz is still hoping to get time to finish some prototype patches for his RFC on variable-sized register classes. In the ideal case, the RISCV backend will build upon these patches and act as a demonstrator to prove the approach. However, if it looks like that isn't going to happen any time soon then I'll bite the bullet and add in all the necessary 64-bit code paths and duplicated instruction definitions.

Diff Detail

Event Timeline

asb created this revision.Feb 14 2017, 4:56 AM
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:27 AM

Hi Alex. Yes, that work is still ongoing. I had to rewrite the type inference from scratch in CodeGenDAGPatterns.cpp and that part works, at least in the sense of preserving the existing behavior (i.e. for patterns that do not use variable classes). What's left is to change the TargetRegisterClass structure and the register info emitter. This is taking time since now it has a much wider scope than it had in the first approximation.

asb updated this revision to Diff 110352.Aug 9 2017, 4:17 AM

This patch update:

  • No longer marks X2 (the stack pointer) as a callee-saved register. It is manipulated directly in the prolog and epilog
  • Adds a default SelectionDAGTargetInfo to RISCVSubtarget.
  • Fixes several minor formatting issues
theraven added inline comments.Aug 9 2017, 4:33 AM
lib/Target/RISCV/RISCVInstrInfo.td
152

This style is followed on the existing back ends to varying degrees, but I find that the mixing of instruction and pattern definitions makes it harder to follow the code when debugging. The places where these are separate are usually a lot easier to spot mistakes (as a trivial example here, the instruction that will be selected for an OR DAG node is defined either on line 145 or on line 172, depending on the operand of the node. Separating these definitions out [ideally into a separate RISCVPatterns.td file that's included] from the instruction definitions and placing them next to each other in the source makes it a lot easier to understand the logic and cleanly separates concerns).

asb added inline comments.Aug 9 2017, 5:01 AM
lib/Target/RISCV/RISCVInstrInfo.td
152

This is an interesting idea. Instruction definitions are ordered to allow easy cross-referencing with the RISC-V specification, but as you point out this means that code generation patterns aren't always grouped in a natural way. The only downside from separating patterns is you can't see at a glance if a pattern might be missing (e.g. no pattern was given for ori) - but that would be caught by tests anyway.

I'll have a play and see what such a factoring would look like.

asb updated this revision to Diff 111629.Aug 18 2017, 1:49 AM

Refreshed patch after auditing for inappropriate use of llvm_unreachable. I've trialled separating out patterns from instruction definitions as suggested by David, and will post more on this shortly.

asb updated this revision to Diff 112443.Aug 23 2017, 2:17 PM

This patch has been updated to split out instruction definitions from patterns, as discussed here and on llvm-dev. The RISCV backend now compiles with guessInstructionProperties=0. It is now dependent on D37065, which fixes the target independent TargetOpcode::* instructions to work when this option is enabled.

mgrang added a subscriber: mgrang.Sep 6 2017, 5:32 PM
mgrang added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
46

Period at end of comment.

67

This currently will always report error. I guess you intend to add more cases in future?

73

Period at end of comment.

92

I think it would be better to omit the braces in case there is a single statement inside if. Would make code more compact, no?

I had the same observation about your other RISCV patches.

asb updated this revision to Diff 114140.Sep 7 2017, 3:32 AM

Address review comments from @mgrang (thanks!).

asb marked 3 inline comments as done.Sep 7 2017, 3:34 AM
asb added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
67

Given this is a patch series, I tend to prefer to leave the 'scaffolding' for future patches to build upon. The next patch in this series starts to flesh out LowerOperation.

kparzysz added inline comments.Sep 7 2017, 6:12 AM
lib/Target/RISCV/RISCVISelDAGToDAG.cpp
49

I'm curious---do you expect Select to be called with a machine node? Normally that should not happen.

lib/Target/RISCV/RISCVRegisterInfo.cpp
49

You could use markSuperRegs to make sure that all super-registers of a reserved register are also marked as reserved. It may (or may not) make sense to add assert(checkAllSuperRegsMarked(Reserved)) too.

asb added inline comments.Sep 7 2017, 7:09 AM
lib/Target/RISCV/RISCVISelDAGToDAG.cpp
49

Select gets called when a node has been lowered using lowerExternalSymbol or lowerGlobalAddress. It's possible I'm missing a way to avoid this, though I can see that the x86 backend also has machine nodes reaching Select (verified by introducing an assert and running CodeGen/X86 tests). The same seems to be true of Hexagon.

kparzysz added inline comments.Sep 7 2017, 7:46 AM
lib/Target/RISCV/RISCVISelDAGToDAG.cpp
49

You're right---Hexagon has that too, and it actually gets called with machine nodes. Never mind, I guess... :)

psnobl added a subscriber: psnobl.Sep 8 2017, 11:03 AM
psnobl added inline comments.Sep 8 2017, 12:10 PM
lib/Target/RISCV/RISCVISelLowering.cpp
61

Is there a special reason why the value is the same for both cases? I would expect to see much lower numbers for when we optimize for size - 16 for example.

psnobl added inline comments.Sep 11 2017, 3:15 AM
lib/Target/RISCV/RISCVCallingConv.td
15

Shouldn't there also be promotion of i8/i16 values to i32 (similar to the argument passing below)?

lib/Target/RISCV/RISCVISelDAGToDAG.cpp
46

You sometimes use errs() and sometimes dbgs() for these debug printouts. This should be unified to dbgs().

asb updated this revision to Diff 115577.Sep 17 2017, 8:45 AM
asb marked 6 inline comments as done.

Update based on review comments (thanks!).

lib/Target/RISCV/RISCVCallingConv.td
15

Seems to be unnecessary (later patches have tests covering returns of value types < i32, and the full gcc torture suite passes at all optimisation levels with my full patchset)., I try not to add extra code unless I can demonstrate its effect with a test.

lib/Target/RISCV/RISCVISelLowering.cpp
61

Good catch, I've removed these. They were left over from when I was exploring a runtime failure.

reames accepted this revision.Oct 2 2017, 8:50 PM
reames added a subscriber: reames.

LGTM so as to unblock patches. I don't see anything obviously wrong with these patches though I might be missing details as this is an area I'm not hugely familiar with.

As a side note, the amount of boilerplate needed here is disturbing. I'd have expected us to be able to avoid that through tablegen, or well structured class structures or something.

lib/Target/RISCV/RISCVISelLowering.cpp
96

minor: use early return for the non-reg case

110

minor: please put the default at the top to make it obvious.

144

for-each loop?

lib/Target/RISCV/RISCVInstrInfo.td
191

Please, please, please do not post whitespace diffs. Feel free to commit these without review.

lib/Target/RISCV/RISCVMCInstLower.cpp
32

for-each

This revision is now accepted and ready to land.Oct 2 2017, 8:50 PM
reames added a subscriber: test.Oct 2 2017, 8:54 PM
reames added inline comments.
test/CodeGen/RISCV/alu.ll
11

if we'd wanted to be extremely pedantic about splitting up patches into the minimal possible functionality, you could have landed support for *just* the return statement (i.e. not the ALU bits). That would have let you split out a tiny bit of code, but not enough to be worth splitting now.

i.e. test cases of the form:
define i32 @test(i32 %a) {

ret i32 %a

}

asb updated this revision to Diff 119597.Oct 19 2017, 10:03 AM
asb marked 3 inline comments as done.

Address review comments from Philip (thanks!).

Note that there have been some other changes in the .td files and and tests, but these are purely cosmetic (formatting patterns etc in a consistent fashion that will match support for various RISC-V extensions, and naming CHECK lines in a way that will reduce churn when adding RV64 support).

This revision was automatically updated to reflect the committed changes.