Page MenuHomePhabricator

[ARM] Fix Machine Outliner LDRD/STRD handling in Thumb mode
ClosedPublic

Authored by yroux on May 26 2021, 7:20 AM.

Details

Summary

This is a fix for PR50481

Immediate values for AddrModeT2_i8s4 are already scaled in MCinst operand, change the number of bits
and scale factor to reflect that state when checking stack offset status. AddrModeT2_i7s[2|4] also have
this particularity but since MVE instructions are outlined, just move these cases to the unhandled ones.

Diff Detail

Event Timeline

yroux created this revision.May 26 2021, 7:20 AM
yroux requested review of this revision.May 26 2021, 7:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 7:20 AM
efriedma added inline comments.May 27 2021, 2:55 PM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5987

Maybe assert "Fixup" is a multiple of 4?

chill added a subscriber: chill.Jun 2 2021, 5:53 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6001

Can't we instead return false; here and get rid of enumerating all unsupported addressing modes on line 5933?

yroux added inline comments.Jun 2 2021, 6:21 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
5987

ok, and I'll change the check line 6005 into an assert

6001

hmm, yes I agree it'll make it easier to read

yroux added inline comments.Jun 2 2021, 6:47 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6001

The issue is that getting the offset value line 5948 can only be done if the addressing mode has one, so we can't get rid of the checking line 5933. I already planned to rework this part and extract it since that kind of logic is used in multiple places in the backend, but what about keeping it as it for this bugfix ?

yroux updated this revision to Diff 349257.Jun 2 2021, 6:50 AM
chill added inline comments.Jun 2 2021, 7:46 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6001

Agree, it's a separate concern.

nagisa added a subscriber: nagisa.Jun 5 2021, 5:10 AM
yroux added a comment.Jun 7 2021, 6:03 AM

@efriedma, @chill is this patch ok for you ? (it'd be great to have the issue fixed into 12.0.1 release)

chill added a comment.Jun 7 2021, 6:28 AM

@efriedma, @chill is this patch ok for you ? (it'd be great to have the issue fixed into 12.0.1 release)

LGTM, just if you intend to reuse the summary as a commit message "... since MVE instructions are *not* outlined...".

yroux added a comment.Jun 7 2021, 6:43 AM

LGTM, just if you intend to reuse the summary as a commit message "... since MVE instructions are *not* outlined...".

OK will do, thanks

chill accepted this revision.Jun 9 2021, 4:47 AM
This revision is now accepted and ready to land.Jun 9 2021, 4:47 AM
This revision was landed with ongoing or failed builds.Jun 9 2021, 6:37 AM
This revision was automatically updated to reflect the committed changes.