Page MenuHomePhabricator

[SVE] Fix bad FixedVectorType cast in simplifyDivRem
ClosedPublic

Authored by ctetreau on Jun 15 2020, 10:27 AM.

Details

Summary

simplifyDivRem attempts to walk a VectorType elementwise. Ensure that it
only does so for FixedVectorType

Diff Detail

Event Timeline

ctetreau created this revision.Jun 15 2020, 10:27 AM
Herald added a project: Restricted Project. · View Herald Transcript

Do you have a regression test with a scalable vector for this pattern?

This codepath is tested for scalable vectors by LLVM.Transforms/InstCombine::udiv-pow2-vscale.ll

david-arm accepted this revision.Jun 16 2020, 3:36 AM
This revision is now accepted and ready to land.Jun 16 2020, 3:36 AM

@spatel Are you satisfied with the existing test? May I merge this?

@spatel Are you satisfied with the existing test? May I merge this?

I'm confused - if the test is already there, but it doesn't change with this patch, then what does the failure/bug look like?

I'm confused - if the test is already there, but it doesn't change with this patch, then what does the failure/bug look like?

The issue that this patch is solving is that this code calls getNumElements through a base VectorType pointer, which is going to be removed soon. getAggregateElement() will only return a value for a scalable vector constant if it is zeroinitializer or undef. Both of these cases are handled before the lines changed by this patch. The test I listed above hits this code path. What currently happens is getAggregateElement returns nullptr for each of 0 .. VTy->getElementCount().Min, and then the block exits. Essentially, this is NFC.

spatel accepted this revision.Jun 16 2020, 12:41 PM

I'm confused - if the test is already there, but it doesn't change with this patch, then what does the failure/bug look like?

The issue that this patch is solving is that this code calls getNumElements through a base VectorType pointer, which is going to be removed soon. getAggregateElement() will only return a value for a scalable vector constant if it is zeroinitializer or undef. Both of these cases are handled before the lines changed by this patch. The test I listed above hits this code path. What currently happens is getAggregateElement returns nullptr for each of 0 .. VTy->getElementCount().Min, and then the block exits. Essentially, this is NFC.

OK, thanks for explaining. LGTM.

This revision was automatically updated to reflect the committed changes.