As a reminder, the philosophy here is to avoid copying large portions of codes from other backends, and only add new code paths when they are both needed and can be tested. As such, even though there's nothing preventing this patch from supporting compilation with PIC, it's something that should come later. I feel this is a logical "next testable unit" after the initial ALU codegen.
Details
Diff Detail
Event Timeline
The patch has been updated with a new wide-mem.ll test case, several minor formatting issues have been fixed, and the mem.ll test has been updated to reflect upstream changes in instruction schedules.
lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
94 | Shouldn't this be report_fatal_error ? |
Instructions definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html
Update based on review comments (thanks!). This version of the patch also adds (and tests) support for sextload/zextload/anyextload of an i1 value.
lib/Target/RISCV/RISCV.h | ||
---|---|---|
19 | this include is not really needed when you have "class MCInst;" below. | |
lib/Target/RISCV/RISCVISelLowering.cpp | ||
77 | I'm honestly confused. Why is LowerOperation CamelCase, but lowerGlobalAddress is camelCase? I've seen this inconsistency of function naming elsewhere too, and all I can think of is that LLVM is changing to camelCase function code style just very very slowly. Is this correct? Anyway, I'll leave the decision up to you, but these two functions have almost identical signature and a different style, that doesn't seem right. | |
lib/Target/RISCV/RISCVMCInstLower.cpp | ||
16–18 | This file now seems to be working perfectly fine with just these includes: |
lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
77 | Yes, LLVM is very slowly moving to camelCase for function names. LowerOperation is called from target-independent code, so we're stuck with the old naming style for now. |
test/CodeGen/RISCV/alu.ll | ||
---|---|---|
164 | These tests have nothing to do with memory access. You should separate a patch which implements this support. | |
test/CodeGen/RISCV/mem.ll | ||
140 | It looks like there's non-trivial support required specifically for globals. This should probably be separated into it's own patch. You might also be able to start with a simpler (orthogonal) test. Consider: return i32* @G } |
Updated to split constant materialization to a preceding patch (D39101) and global addresses to a succeeding patch (to be posted shortly).
LGTM
Thanks for splitting. Tracking the pieces is now much easier.
lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
39 | This looks like it can just be an assert. |
Thanks for helping to unblock this Philip. I was holding off on committing as Chandler was hoping to corral another backend maintainer into giving a second opinion. Unfortunately it looks like that person didn't have time, so I'm going ahead and committing.
this include is not really needed when you have "class MCInst;" below.
Same for including MachineOperand.h, wouldn't "class MachineOperand;" be sufficient?