Page MenuHomePhabricator

[AVR] Add a majority of the backend code
ClosedPublic

Authored by dylanmckay on Mar 4 2016, 8:17 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dylanmckay updated this revision to Diff 49871.Mar 4 2016, 8:17 PM
dylanmckay retitled this revision from to [AVR] Add a majority of the backend code.
dylanmckay updated this object.
dylanmckay added reviewers: arsenm, hfinkel, dsanders.
arsenm added inline comments.Apr 13 2016, 7:22 PM
lib/Target/AVR/AVRFrameLowering.h
9–12 ↗(On Diff #49871)

Remove empty comment block

30 ↗(On Diff #49871)

Pointless comment

lib/Target/AVR/AVRISelLowering.h
119–123 ↗(On Diff #49871)

C++ style comments

lib/Target/AVR/AVRInstrInfo.cpp
38–43 ↗(On Diff #49871)

These are not OK

lib/Target/AVR/AVRInstrInfo.h
23 ↗(On Diff #49871)

Missing undef?

lib/Target/AVR/AVRSubtarget.h
68 ↗(On Diff #49871)

this->?

dylanmckay updated this revision to Diff 53861.Apr 15 2016, 2:17 AM
dylanmckay edited edge metadata.

First round of code review

dylanmckay marked 5 inline comments as done.Apr 15 2016, 2:21 AM
dylanmckay added inline comments.
lib/Target/AVR/AVRInstrInfo.cpp
39–44 ↗(On Diff #53861)

That wasn't meant to make it in here, it's gone now.

lib/Target/AVR/AVRInstrInfo.h
24 ↗(On Diff #53861)

Nice catch

dylanmckay marked 5 inline comments as done.Apr 15 2016, 2:22 AM

Back to you Matt

arsenm edited edge metadata.Apr 27 2016, 11:56 AM

LGTM with some style nits, plus the pass setup seems to be adding a couple of passes I don't see being added

lib/Target/AVR/AVRFrameLowering.h
19–21 ↗(On Diff #53861)

C++ style comments

lib/Target/AVR/AVRISelLowering.h
39 ↗(On Diff #53861)

Why not all caps like the others?

82–83 ↗(On Diff #53861)

I think it's best to not duplicate the comments on overrides. The base class header comment is more likely to be kept up to date

lib/Target/AVR/AVRInstrInfo.cpp
69 ↗(On Diff #53861)

brace on case lines

390 ↗(On Diff #53861)

Question mark?

462 ↗(On Diff #53861)

static_cast

lib/Target/AVR/AVRInstrInfo.h
29–33 ↗(On Diff #53861)

C++ comments

lib/Target/AVR/AVRRegisterInfo.cpp
57–69 ↗(On Diff #53861)

Some comments about why these registers are reserved might be helpful

171 ↗(On Diff #53861)

Fallthrough

lib/Target/AVR/AVRRegisterInfo.h
23–25 ↗(On Diff #53861)

C++ comments. Repeat for everywhere else

lib/Target/AVR/AVRSubtarget.cpp
37 ↗(On Diff #53861)

Do these actually initialize to 0? I don't think I've seen empty parens like this before

lib/Target/AVR/AVRSubtarget.h
38 ↗(On Diff #53861)

Extra lne

48 ↗(On Diff #53861)

return on new line like the others here

lib/Target/AVR/AVRTargetMachine.cpp
29 ↗(On Diff #53861)

This is only one place, so you can just move the literal there

32 ↗(On Diff #53861)

static, Get->get

35 ↗(On Diff #53861)

No return after else

97 ↗(On Diff #53861)

Don't see this

104 ↗(On Diff #53861)

I don't see this file?

lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.h
23–25 ↗(On Diff #53861)

Alphabetize

dylanmckay marked 19 inline comments as done.May 6 2016, 3:14 AM
dylanmckay added inline comments.
lib/Target/AVR/AVRRegisterInfo.cpp
57–69 ↗(On Diff #53861)

Agree

dylanmckay updated this revision to Diff 56393.May 6 2016, 3:15 AM
dylanmckay marked an inline comment as done.
dylanmckay edited edge metadata.

Second round of Matt's code review

This revision was automatically updated to reflect the committed changes.

Thanks for the review Matt :)