Page MenuHomePhabricator

[AArch64] Fix assertion failure on widened f16 BUILD_VECTOR
ClosedPublic

Authored by bryanpkc on Aug 2 2018, 3:33 PM.

Details

Summary

Ensure that NormalizedBuildVector returns a BUILD_VECTOR with operands of the
same type. This fixes an assertion failure in VerifySDNode.

Diff Detail

Repository
rL LLVM

Event Timeline

bryanpkc created this revision.Aug 2 2018, 3:33 PM
SjoerdMeijer added inline comments.Aug 3 2018, 2:37 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
6896 ↗(On Diff #158851)

Doesn't look like you're testing this; add a test for this?

test/CodeGen/AArch64/arm64-build-vector.ll
43 ↗(On Diff #158851)

Nit: perhaps move some of these explanations to the implementation in NormalizeBuildVector? But definitely a brief explanation of the test would be good.

bryanpkc added inline comments.Aug 3 2018, 10:21 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
6896 ↗(On Diff #158851)

Actually the test is exercising this path; widening adds i16 UNDEF nodes to a vector, but when normalizing the vector, they were left alone and not promoted.

The next block is in fact a preemptive fix, for which I couldn't construct a test case. However, zero-extending a 16-bit or smaller operand in an integer BUILD_VECTOR to 32 bits seems pretty safe to me, and is consistent with what we already do for constant operands in this function.

test/CodeGen/AArch64/arm64-build-vector.ll
43 ↗(On Diff #158851)

OK, will do.

bryanpkc updated this revision to Diff 159092.Aug 3 2018, 2:21 PM

Removed impossible code path from NormalizeBuildVector.

SjoerdMeijer accepted this revision.Aug 6 2018, 1:56 AM

Thanks, looks reasonable to me.

This revision is now accepted and ready to land.Aug 6 2018, 1:56 AM
This revision was automatically updated to reflect the committed changes.