This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Add the AVR frame lowering code
ClosedPublic

Authored by dylanmckay on Sep 28 2016, 9:06 AM.

Details

Summary

This allows AVR to lower frames into assembly code.

Event Timeline

dylanmckay updated this revision to Diff 72847.Sep 28 2016, 9:06 AM
dylanmckay retitled this revision from to [AVR] Add the AVR frame lowering code.
dylanmckay updated this object.
dylanmckay added reviewers: kparzysz, arsenm.
japaric added a subscriber: japaric.Oct 3 2016, 6:46 AM
kparzysz added inline comments.Oct 4 2016, 11:33 AM
lib/Target/AVR/AVRFrameLowering.cpp
50

Isn't this last condition backwards? Also, why use isUInt?

60

Subtarget should be obtained from a MachineFunction, since it can be different every time.
const AVRInstrInfo &TII = *MF.getSubtarget<AVRSubtarget>().getInstrInfo();

149

Check for MBBI being MBB.end(). Also, getDesc() is not necessary.

157

Same note about Subtarget.

162

These calls do not modify MBBI.

183

That's just esthetics, but most of these parentheses are not necessary.

240

Subtarget from MF.

280

And here...

282

Make it a range-based loop?

302

"auto" is ok for iterators.

306

Opcode is unsigned.

328

...

365

...

442

Is this "if (x - y)"? if (x != y) would be clearer.

464

Make it a range-based loop.

467

Same here.

476

And here.

516

...

530

And here.

dylanmckay updated this revision to Diff 73599.Oct 5 2016, 1:19 AM
dylanmckay marked 19 inline comments as done.

Code review from Krzysztof

  • Converted a bunch of loops into range syntax
  • Get AVRSubtarget from the current machine function

Thanks for the comprehensive review Krzysztof :)

lib/Target/AVR/AVRFrameLowering.cpp
50

Isn't this last condition backward

Nice catch :)

Also, why use isUInt?

I feel like it's more descriptive than Size > 63, which seems arbitrary - if we're checking if it can fit into 6 bits, it gives us a little more context on why it's there.

kparzysz accepted this revision.Oct 5 2016, 4:45 AM
kparzysz edited edge metadata.
This revision is now accepted and ready to land.Oct 5 2016, 4:45 AM
This revision was automatically updated to reflect the committed changes.