[PATCH] D50202: [AArch64] Fix assertion failure on widened f16 BUILD_VECTOR

[Bryan Chan via Phabricator via llvm-commits]

bryanpkc added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:6896
       Lane = DAG.getConstant(LowBits.getZExtValue(), dl, MVT::i32);
+    } else if (Lane.getNode()->isUndef()) {
+      Lane = DAG.getUNDEF(MVT::i32);
----------------
SjoerdMeijer wrote:
> Doesn't look like you're testing this; add a test for this?
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.


================
Comment at: test/CodeGen/AArch64/arm64-build-vector.ll:43
+
+; The lowering of an f16 BUILD_VECTOR attempts to optimize it by building an
+; equivalent integer vector and then BITCAST-ing the result. This case checks
----------------
SjoerdMeijer wrote:
> Nit: perhaps move some of these explanations to the implementation in NormalizeBuildVector? But definitely a brief explanation of the test would be good.
OK, will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D50202