This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Add a majority of the backend code
ClosedPublic

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

Diff Detail

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
10–13

Remove empty comment block

31

Pointless comment

lib/Target/AVR/AVRISelLowering.h
120–124

C++ style comments

lib/Target/AVR/AVRInstrInfo.cpp
39–44

These are not OK

lib/Target/AVR/AVRInstrInfo.h
24

Missing undef?

lib/Target/AVR/AVRSubtarget.h
69

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

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

lib/Target/AVR/AVRInstrInfo.h
24

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

C++ style comments

lib/Target/AVR/AVRISelLowering.h
39

Why not all caps like the others?

82–83

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

brace on case lines

390

Question mark?

462

static_cast

lib/Target/AVR/AVRInstrInfo.h
29–33

C++ comments

lib/Target/AVR/AVRRegisterInfo.cpp
57–69

Some comments about why these registers are reserved might be helpful

171

Fallthrough

lib/Target/AVR/AVRRegisterInfo.h
23–25

C++ comments. Repeat for everywhere else

lib/Target/AVR/AVRSubtarget.cpp
37

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

lib/Target/AVR/AVRSubtarget.h
38

Extra lne

48

return on new line like the others here

lib/Target/AVR/AVRTargetMachine.cpp
29

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

32

static, Get->get

35

No return after else

97

Don't see this

104

I don't see this file?

lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.h
23–25

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

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 :)