This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Fix missed immediate selection due to mishandling of signedness
ClosedPublic

Authored by bsmith on May 4 2021, 7:09 AM.

Details

Summary

The complex selection pattern for add/sub shifted immediates is
incorrect in it's handling of incoming constant values, in that it
does not properly anticipate the values to be signed extended to
32-bits.

Co-authored-by: Graham Hunter <graham.hunter@arm.com>

Diff Detail

Event Timeline

bsmith created this revision.May 4 2021, 7:09 AM
bsmith requested review of this revision.May 4 2021, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 7:09 AM
efriedma added inline comments.May 4 2021, 11:19 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3121

Does checking ((int32_t)ImmVal >= -32768) && ((int32_t)ImmVal <= 32512)) actually do anything? All 16-bit immediates should be in that range.

bsmith added inline comments.May 5 2021, 3:45 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3121

It possibly doesn't, however the thing that makes me nervous here is that the incoming CNode value is an i32 so could in theory (if something has gone wrong?) have a value outside of that range. It might be best to do it like this if nothing else to prevent an issue further up the pipeline propagating into a codegen fault here?

david-arm added inline comments.May 5 2021, 3:55 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3121

I guess you could possibly remove those checks and replace them with an assert?

Matt added a subscriber: Matt.May 5 2021, 4:59 AM
bsmith updated this revision to Diff 343005.May 5 2021, 5:04 AM
bsmith marked 2 inline comments as done.
  • Fix clang format issues
  • Move range check to be an assert since it should always be true
peterwaller-arm added inline comments.May 6 2021, 3:20 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3122

Suggestion: Use shift operation since this is about bit manipulation, and it's consistent with other uses nearby.

bsmith updated this revision to Diff 343418.May 6 2021, 8:27 AM
  • Use shift in place of divide for consistency.
peterwaller-arm accepted this revision.May 12 2021, 7:10 AM
This revision is now accepted and ready to land.May 12 2021, 7:10 AM