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
40–45

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

lib/Target/AVR/AVRInstrInfo.h
25

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
20–22

C++ style comments

lib/Target/AVR/AVRISelLowering.h
40

Why not all caps like the others?

83–84

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
70

brace on case lines

391

Question mark?

463

static_cast

lib/Target/AVR/AVRInstrInfo.h
30–34

C++ comments

lib/Target/AVR/AVRRegisterInfo.cpp
58–70

Some comments about why these registers are reserved might be helpful

172

Fallthrough

lib/Target/AVR/AVRRegisterInfo.h
24–26

C++ comments. Repeat for everywhere else

lib/Target/AVR/AVRSubtarget.cpp
38

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

lib/Target/AVR/AVRSubtarget.h
39

Extra lne

49

return on new line like the others here

lib/Target/AVR/AVRTargetMachine.cpp
30

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

33

static, Get->get

36

No return after else

92

Don't see this

99

I don't see this file?

lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.h
24–26

Alphabetize

dylanmckay marked 19 inline comments as done.May 6 2016, 3:14 AM
dylanmckay added inline comments.
lib/Target/AVR/AVRRegisterInfo.cpp
58–70

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