This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Add instruction definitions
ClosedPublic

Authored by dylanmckay on Dec 21 2015, 3:50 PM.

Diff Detail

Event Timeline

dylanmckay updated this revision to Diff 43410.Dec 21 2015, 3:50 PM
dylanmckay retitled this revision from to [AVR] Add instruction definitions.
dylanmckay updated this object.
arsenm added inline comments.Dec 21 2015, 9:52 PM
lib/Target/AVR/AVRInstrFormats.td
57

I think you should also set isCodeGenOnly as well

lib/Target/AVR/AVRInstrInfo.td
59

These custom shift nodes look strange. It looks like the shift amount is passed in an implicit register, but you aren't adding glue to the node for the copy into it

1822

I would recommend putting things like the instruction bits and implicit uses and defs generally in a new class to avoid huge numbers of let blocks

1891

Should the pattern inputs just be i8 instead of the register class?

1962

Can you add an xfailed test for this

rjordans added inline comments.
lib/Target/AVR/AVRInstrInfo.td
59

AVR only has shift-by-1 kind of operations so they don't get a shift amount specified through a register

dylanmckay marked 3 inline comments as done.Dec 23 2015, 1:18 AM
dylanmckay added inline comments.
lib/Target/AVR/AVRInstrInfo.td
59

@rjordans is right, which also makes for repetitive code generated for shifts bigger than one bit.

1962

Not yet, as the lowering code has not been merged currently. I will add it to a TODO list in lib/Target/AVR however.

dylanmckay marked 2 inline comments as done.Dec 23 2015, 1:20 AM
dylanmckay updated this revision to Diff 43513.Dec 23 2015, 1:21 AM

Set isCodeGenOnly for Pseudo instructions

agnat added a subscriber: agnat.Dec 23 2015, 8:18 AM
dylanmckay updated this revision to Diff 43582.Dec 23 2015, 8:10 PM

Factor out common psuedo instruction properties

Now we have different classes of pseudo instructions, reducing
duplication of code.

dylanmckay updated this revision to Diff 43583.Dec 23 2015, 8:24 PM

Use i8/i16 instead of register classes in patterns

dylanmckay marked 4 inline comments as done.Dec 23 2015, 8:26 PM

@arsenm Fixed up your points, the pseudo class one was especially nice.

Ready for another round of review.

Ping.

rjordans added inline comments.Jan 30 2016, 5:25 AM
lib/Target/AVR/AVRInstrInfo.td
873

This instruction should be defined differently:

def CPIRdK : FRdK<0b0011,
                  (outs),
                  (ins GPR8:$src, i8imm:$k),
                  "cpi\t$src, $k",
                  [(AVRcmp GPR8:$src, imm:$k), (implicit SREG)]>;
dylanmckay updated this revision to Diff 46501.Jan 31 2016, 5:24 PM

Fix definition of 'cpi' instruction

Also add ISel pattern.

dylanmckay marked an inline comment as done.Jan 31 2016, 5:26 PM
dylanmckay added inline comments.
lib/Target/AVR/AVRInstrInfo.td
874

Good catch - thanks Roel.

dylanmckay marked an inline comment as done.Feb 5 2016, 2:42 PM

Ping.

arsenm accepted this revision.Feb 9 2016, 10:27 AM
arsenm edited edge metadata.

LGTM.

The tablegen formatting looks weird to me, but there don't seem to be any consistently followed standards there.

lib/Target/AVR/AVRInstrInfo.td
65

Should be capitalized and end with a period

650

You should just be able to use i8:$src instead of the specific register class in these patterns

This revision is now accepted and ready to land.Feb 9 2016, 10:27 AM
dylanmckay marked an inline comment as done.
dylanmckay edited edge metadata.

Fixed Matt's code review points

dylanmckay closed this revision.Feb 10 2016, 12:59 AM
dylanmckay marked 2 inline comments as done.Feb 10 2016, 1:00 AM