This is an archive of the discontinued LLVM Phabricator instance.

[RISCV 12/n] Codegen support for memory operations
ClosedPublic

Authored by asb on Feb 14 2017, 5:09 AM.

Details

Summary

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.

Diff Detail

Event Timeline

asb created this revision.Feb 14 2017, 5:09 AM
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:28 AM
asb updated this revision to Diff 110355.Aug 9 2017, 4:21 AM

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.

majnemer added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
94

Shouldn't this be report_fatal_error ?

asb updated this revision to Diff 111630.Aug 18 2017, 1:51 AM
asb marked an inline comment as done.

Fix inappropriate use of llvm_unreachable (thanks @majnemer!).

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

Instructions definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

mgrang added a subscriber: mgrang.Sep 6 2017, 5:34 PM
mgrang added inline comments.
lib/Target/RISCV/RISCVMCInstLower.cpp
85

Not sure about the indentation of this line. Can you run clang-format?

86

Same here.

asb updated this revision to Diff 114145.Sep 7 2017, 4:01 AM
asb marked 2 inline comments as done.

Fix indentation (thanks @mgrang!).

asb added inline comments.Sep 7 2017, 4:02 AM
lib/Target/RISCV/RISCVMCInstLower.cpp
85

This line was produced by clang-format.

86

This line was also produced by clang-format, however I notice indentation of this whole function was was broken during some refactoring. Fixed.

psnobl added a subscriber: psnobl.Sep 8 2017, 11:46 AM
kparzysz added inline comments.
lib/Target/RISCV/RISCVISelLowering.h
32

Please make this a reference, since Subtarget cannot be nullptr.

lib/Target/RISCV/RISCVInstrInfo.td
294

Line just a bit too long.

309

This line is too long.

asb updated this revision to Diff 115578.Sep 17 2017, 8:53 AM
asb marked 3 inline comments as done.

Update based on review comments (thanks!). This version of the patch also adds (and tests) support for sextload/zextload/anyextload of an i1 value.

glasnak added inline comments.Sep 22 2017, 8:58 AM
lib/Target/RISCV/RISCV.h
19

this include is not really needed when you have "class MCInst;" below.
Same for including MachineOperand.h, wouldn't "class MachineOperand;" be sufficient?

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:
#include "RISCV.h"
#include "MCTargetDesc/RISCVMCExpr.h"
#include "llvm/CodeGen/AsmPrinter.h"
#include "llvm/MC/MCAsmInfo.h"
#include "llvm/MC/MCContext.h"
please remove the others.

asb added inline comments.Sep 22 2017, 9:00 AM
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.

asb updated this revision to Diff 116946.Sep 28 2017, 2:14 AM
asb marked 4 inline comments as done.

Clean up #includes.

reames requested changes to this revision.Oct 18 2017, 4:57 PM
reames added subscribers: test, reames.
reames added inline comments.
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:
define i32* @test() {

return i32* @G

}

This revision now requires changes to proceed.Oct 18 2017, 4:57 PM
asb updated this revision to Diff 119603.Oct 19 2017, 11:24 AM
asb edited edge metadata.
asb edited the summary of this revision. (Show Details)

Updated to split constant materialization to a preceding patch (D39101) and global addresses to a succeeding patch (to be posted shortly).

reames accepted this revision.Oct 20 2017, 3:57 PM

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.

This revision is now accepted and ready to land.Oct 20 2017, 3:57 PM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Nov 8 2017, 4:45 AM

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.