Page MenuHomePhabricator

[GlobalISel] Fix a bug in LegalizeRuleSet::clampMaxNumElements
ClosedPublic

Authored by volkan on Oct 25 2018, 2:55 PM.

Details

Summary

This function was causing a crash when MaxElements == 1 because
it was trying to create a single element vector type.

Diff Detail

Repository
rL LLVM

Event Timeline

volkan created this revision.Oct 25 2018, 2:55 PM
dsanders accepted this revision.Nov 1 2018, 10:04 AM

LGTM with a couple testing nits

lib/Target/AArch64/AArch64LegalizerInfo.cpp
173 ↗(On Diff #171202)

It's not for this patch, but we ought to have a version of clampMaxNumElements() that applies to every vector except for those on a whitelist. Maybe, something like:

.clampMaxNumElements(0, except({s32}), 1)
test/CodeGen/AArch64/GlobalISel/legalize-load-fewerElts.mir
5–6 ↗(On Diff #171202)

It's better to test the end result rather than the particular action the legalizer takes for this step. Can you drop the -debug-only=legalizer from the run line and test with the output you get without that?

This revision is now accepted and ready to land.Nov 1 2018, 10:04 AM
volkan added inline comments.Nov 1 2018, 10:45 AM
lib/Target/AArch64/AArch64LegalizerInfo.cpp
173 ↗(On Diff #171202)

I agree. I think we should add the opposite as well, a function that takes a list of types. For example:

.clampMaxNumElements(0, {s16, s32, s64, ...}, 1)
test/CodeGen/AArch64/GlobalISel/legalize-load-fewerElts.mir
5–6 ↗(On Diff #171202)

That was my plan, but D53728 is required to do that. I'll update the tests in D53728 after committing this.

dsanders added inline comments.Nov 1 2018, 11:56 AM
test/CodeGen/AArch64/GlobalISel/legalize-load-fewerElts.mir
5–6 ↗(On Diff #171202)

Ok. That sounds good to me

This revision was automatically updated to reflect the committed changes.