Page MenuHomePhabricator

[AVR] Fix expansion of NEGW
ClosedPublic

Authored by aykevl on Feb 20 2021, 4:39 PM.

Details

Summary

The previous expansion used SBCI, which is incorrect because the NEGW pseudo instruction accepts a DREGS operand (2xGPR8) and SBCI only allows LD8 registers. One solution could be to correct the NEGW pseudo instruction, but another solution is to use a different instruction (sbc) that does accept a GPR8 register and therefore allows more freedom to the register allocator.

The output now matches avr-gcc for the following code:

int foo(int n) {
    return -n;
}

I've found this issue using the machine instruction verifier: it was complaining about the wrong register class in NEGWRd.mir.

Diff Detail

Event Timeline

aykevl created this revision.Feb 20 2021, 4:39 PM
aykevl requested review of this revision.Feb 20 2021, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 4:39 PM
aykevl edited the summary of this revision. (Show Details)Feb 20 2021, 4:45 PM
benshi001 added inline comments.Feb 20 2021, 9:39 PM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
449–450

this line needs to be deleted, otherwise there might be a crash ?

aykevl added inline comments.Feb 21 2021, 3:35 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
449–450

Why?

benshi001 accepted this revision.Feb 21 2021, 5:49 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
449–450

Ops, I made a mistake. That is not correct.

This revision is now accepted and ready to land.Feb 21 2021, 5:49 AM

Why not commit it? I think this bug fix should be fixed ASAP.

aykevl added a comment.Mar 1 2021, 5:44 PM

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Yes. I appreciate if you commit your patches. I will do my future work based on it (to avoid potential merge conflict between us)

benshi001 added a comment.EditedMar 1 2021, 6:11 PM

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Yes. I appreciate if you commit your patches. I will do my future work based on it (to avoid potential merge conflict between us)

I have marked 4 of your patches as accepted. I appreciate if you commit them, to avoid potential merge confilcts between us, since I am also working on the same part as you are doing.

This revision was landed with ongoing or failed builds.Mar 3 2021, 6:36 AM
This revision was automatically updated to reflect the committed changes.
aykevl added a comment.Mar 3 2021, 6:38 AM

Ok, committed.