Page MenuHomePhabricator

[AArch64] Skip mask checks for masks with an odd number of elements.
ClosedPublic

Authored by fhahn on Apr 15 2019, 5:49 AM.

Details

Summary

Some checks in isShuffleMaskLegal expect an even number of elements,
e.g. isTRN_v_undef_Mask or isUZP_v_undef_Mask, otherwise they access
invalid elements and crash. This patch adds a check to
isShuffleMaskLegal to ensure there is an even number of elements or
exactly one. The case with one element is relevant for matching some dup
cases. Masks with one element will still cause a crash in isTRN_v_undef_Mask and
isUZP_v_undef_Mask, but I am not sure if that is a realistic scenario. I
can add a separate check or an assertion, if required.

I am not entirely sure if the check is overly restrictive, as I am not
too familiar with the patterns here. We could always push it down to the
problematic functions.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Apr 15 2019, 5:49 AM
Gerolf added a subscriber: Gerolf.Apr 16 2019, 3:26 PM

Could you separate size one checks are OK vs. checks that require even powers of 2? That smells like an inconsistency worth calling out explicitly.

Thanks
Gerolf

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7553 ↗(On Diff #195144)

This check could be the early exit at the start of the function.

fhahn updated this revision to Diff 197947.May 3 2019, 3:44 AM

Push even element checks to the impacted functions.

fhahn added a comment.May 3 2019, 3:46 AM

Could you separate size one checks are OK vs. checks that require even powers of 2? That smells like an inconsistency worth calling out explicitly.

Thanks Gerolf. I pushed the checks to the impacted functions, which seems to better limit the impact of the checks. What do you think?

fhahn added a comment.May 20 2019, 2:23 PM

Ping. @Gerolf, are you happy with the update?

This also independently popped up as PR41951

fhahn marked an inline comment as done.May 20 2019, 2:23 PM
fhahn added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7538 ↗(On Diff #197947)

Unrelated change, I'll undo before committing.

dmgreen accepted this revision.May 21 2019, 1:31 AM

LGTM. Thanks for making the fix.

This revision is now accepted and ready to land.May 21 2019, 1:31 AM
This revision was automatically updated to reflect the committed changes.