This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Add AVRMCExpr
ClosedPublic

Authored by dylanmckay on May 20 2016, 6:06 PM.
Tokens
"Doubloon" token, awarded by jtbandes.

Details

Summary

This adds the AVRMCExpr headers and implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

dylanmckay updated this revision to Diff 58023.May 20 2016, 6:06 PM
dylanmckay retitled this revision from to [AVR] Add AVRMCExpr.
dylanmckay updated this object.
dylanmckay added a reviewer: arsenm.
dylanmckay added a subscriber: llvm-commits.
arsenm added inline comments.May 20 2016, 6:28 PM
lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
23 ↗(On Diff #58023)

const char* const?

44–49 ↗(On Diff #58023)

Single quote around single chars

64–65 ↗(On Diff #58023)

No return after else

168 ↗(On Diff #58023)

const char?

176 ↗(On Diff #58023)

nullptr

180–182 ↗(On Diff #58023)

There's an llvm::find_if which takes a range instead of the separate iterators

dylanmckay updated this revision to Diff 58031.May 20 2016, 8:26 PM
dylanmckay marked 5 inline comments as done.

Mat's code review

dylanmckay added inline comments.May 20 2016, 8:28 PM
lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
181–183 ↗(On Diff #58031)

I can't seem to get it working, it tries to call .begin() on the array, which doesn't exist.

rafael edited reviewers, added: grosbach; removed: rafael.Jul 28 2016, 4:54 AM
kparzysz added inline comments.
lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
100 ↗(On Diff #58031)

This should happen before you convert it to an unsigned value.

131 ↗(On Diff #58031)

"AVR::Fixups Kind = AVR::Fixups::LastTargetFixupKind" would be better to indicate an invalid value.

Also, variable names generally start with an upper case.

Also, is it possible to have some basic testcase for this?

dylanmckay updated this revision to Diff 72420.Sep 25 2016, 1:47 AM
dylanmckay marked 2 inline comments as done.

Evaulate negatives using signed integers in AVRMCExpr::evaulateAsInt64

Use AVR::Fixups::LastTargetFixupKind to mark an invalid value

I'm not able to add tests for this just yet, there are still several major parts of the backend missing, so we can't even run llc just yet.

I have unit tests for every single variant of AVRMCExpr, but I can't upstream them until we can actually assemble it.

kparzysz accepted this revision.Sep 26 2016, 3:44 AM
kparzysz added a reviewer: kparzysz.
This revision is now accepted and ready to land.Sep 26 2016, 3:44 AM
This revision was automatically updated to reflect the committed changes.