This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Expand shifts of all types except int8 and int16
ClosedPublic

Authored by Patryk27 on Jul 9 2023, 4:55 AM.

Details

Summary

Currently our AVRShiftExpand pass expands only 32-bit shifts, with the
assumption that other kinds of shifts (e.g. 64-bit ones) are
automatically reduced to 8-bit ones by LLVM during ISel.

However this is not always true and causes problems in the rust-lang runtime.

This commit changes the logic a bit, so that instead of expanding only
32-bit shifts, we expand shifts of all types except 8-bit and 16-bit.

This is not the most optimal solution, because 64-bit shifts can be
expanded to 32-bit shifts which has been deeply optimized.

I've checked the generated code using rustc + simavr, and all shifts
seem to behave correctly.

Spotted in the wild in rustc:

Diff Detail

Event Timeline

Patryk27 created this revision.Jul 9 2023, 4:55 AM
Patryk27 requested review of this revision.Jul 9 2023, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2023, 4:55 AM
benshi001 added inline comments.Jul 10 2023, 7:59 PM
llvm/lib/Target/AVR/AVRShiftExpand.cpp
55–57

I suggest we also exclude int16, since we have already defined pseudo instructions for all int16 shifts, such as

def Asr16 : ShiftPseudo<(outs DREGS
                         : $dst),
                        (ins DREGS
                         : $src, GPR8
                         : $cnt),
                        "# Asr16 PSEUDO", [(set i16
                                            : $dst, (AVRasrLoop i16
                                                     : $src, i8
                                                     : $cnt))]>;

Could you please have a try, that if we change to

if (I.getType() == Type::getInt8Ty(Ctx) || I.getType() == Type::getInt16Ty(Ctx))

can it also fix your issues in rust ?

llvm/test/CodeGen/AVR/shift-expand.ll
121–122

It would be better to also add lshr and ashr for i40 and i24.

Patryk27 updated this revision to Diff 541410.Jul 18 2023, 2:32 AM

Adjust to code review comments

Patryk27 marked 2 inline comments as done.Jul 18 2023, 2:33 AM

I've adjusted the code; adding check for Type::getInt16Ty(Ctx) still generates a correct binary, so even better this way 🙂

LGTM. Thanks! I made some modification to your commit message.

benshi001 accepted this revision.Jul 18 2023, 8:35 PM
This revision is now accepted and ready to land.Jul 18 2023, 8:35 PM
benshi001 retitled this revision from [AVR] Expand all non-8-bit shifts to [AVR] Expand shifts of all types except int8 and int16.Jul 18 2023, 8:44 PM
benshi001 edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Jul 18 2023, 8:57 PM
This revision was automatically updated to reflect the committed changes.