This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix crash for expandMOVImm
ClosedPublic

Authored by Allen on Jan 8 2023, 11:50 PM.

Diff Detail

Event Timeline

Allen created this revision.Jan 8 2023, 11:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 11:50 PM
Allen requested review of this revision.Jan 8 2023, 11:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 11:50 PM
efriedma added inline comments.Jan 9 2023, 9:46 AM
llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
343 ↗(On Diff #487301)

This algorithm doesn't work correctly if BitSize isn't 32 or 64. Probably the code that's creating such an immediate should be fixed.

Allen marked an inline comment as done.Jan 10 2023, 5:56 AM
Allen added inline comments.
llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
343 ↗(On Diff #487301)

Oh, Thanks for your confirm

Allen abandoned this revision.Jan 10 2023, 5:56 AM
Allen marked an inline comment as done.
Allen added inline comments.Jan 11 2023, 5:09 PM
llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
343 ↗(On Diff #487301)

hi @efriedma

For all immediate numbers whose bit size is smaller than or equal to 64, at least four registers can be used for concatenation, ie, the processing of AArch64_IMM::expandMOVImm. Therefore, the current i53 immediate number can be also processed here (may not be the most efficient). Would you please show more detail about where is the algorithm error? thanks.
Allen reclaimed this revision.Jan 12 2023, 5:38 AM
efriedma added inline comments.Jan 12 2023, 10:52 AM
llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
343 ↗(On Diff #487301)

processLogicalImmediate assumes BitSize is a power of two. Loops involving BitSize assume they iterate either twice or four times. Probably other things.

(You could theoretically stick something like "BitSize = BitSize <= 32 ? 32 : 64;" at the beginning of the function, and that would sort-of work, but calling the function with a BitSize that doesn't correspond to the width of a register doesn't make sense.)

Allen updated this revision to Diff 489413.Jan 15 2023, 6:48 PM

Apply the comment

I'd rather stick the bitsize check in the caller, AArch64TargetLowering::isMulAddWithConstProfitable; without that context, it isn't clear why the bitwidth should be modified this way.

Allen updated this revision to Diff 489998.Jan 17 2023, 5:30 PM

Stick the bitsize check in the caller according comment, AArch64TargetLowering::isMulAddWithConstProfitable

This revision is now accepted and ready to land.Jan 18 2023, 11:13 AM
This revision was landed with ongoing or failed builds.Jan 27 2023, 5:26 PM
This revision was automatically updated to reflect the committed changes.