This is an archive of the discontinued LLVM Phabricator instance.

Fix in SelectionDAG::getNode() to not produce illegal BUILD_VECTOR operands
ClosedPublic

Authored by jonpa on Apr 4 2017, 2:26 AM.

Details

Summary

Continued from: https://bugs.llvm.org//show_bug.cgi?id=32422

About the assert in LegalizeOp() that triggered:

llvm/llvm-dev/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:921: void {anonymous}::SelectionDAGLegalize::LegalizeOp(llvm::SDNode*): Assertion `(TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) == TargetLowering::TypeLegal || TLI.isTypeLegal(Op.getValueType()) || Op.getOpcode() == ISD::TargetConstant) && "Unexpected illegal type!"' failed.

I wonder why both getTypeAction() and isTypeLegal() are called here - shouldn't they be equivalent?

Follow up question is then If the patch looks reasonable: is it enough with just isTypeLegal()?

I also removed the check
if (Ops.size() == LegalVT.getVectorNumElements())

since it seems redundant with
if (ISD::isBuildVectorOfConstantSDNodes(N1.getNode()))

Diff Detail

Event Timeline

jonpa created this revision.Apr 4 2017, 2:26 AM
jonpa updated this revision to Diff 94022.Apr 4 2017, 2:48 AM
jonpa retitled this revision from Fix in SelectionDAG::getNode() to not produce BUILD_VECTOR operands to Fix in SelectionDAG::getNode() to not produce illegal BUILD_VECTOR operands.

Test case (which I did not manage to reduce further...)

niravd added inline comments.Apr 4 2017, 11:43 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4143

It doesn't seems like we should have to consider if the type is legal. VT should never have been used.

We should be able to use Op.getValueType().getScalarType() wherever LegalScalarVT is used.

4148

This removal of the redundant check seems reasonable and should probably be done as a separate NFC cleanup.

I wonder why both getTypeAction() and isTypeLegal() are called here - shouldn't they be equivalent?

See https://reviews.llvm.org/rL254653 .

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4138

The getScalarType() here is redundant: the type of N1C must be an integer type.

jonpa updated this revision to Diff 94167.Apr 5 2017, 1:05 AM
jonpa marked 3 inline comments as done.
jonpa added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4143

Changed so the BUILD_VECTOR operand type is unchanged in the new BUILD_VECTOR. I thought first there might be a point in having a legal smaller operand type, but at second thought there seems to be no point as BUILD_VECTOR will truncate and the smaller constant operands should get put into the smaller vector still.

Also removed the call to zextOrTrunc() since we keep the same VT for Op.

4148

OK, will do a separate NFC commit for the redundant check.

niravd accepted this revision.Apr 5 2017, 6:32 AM

LGTM.

This revision is now accepted and ready to land.Apr 5 2017, 6:32 AM
jonpa closed this revision.Apr 5 2017, 6:58 AM

Thanks!

r299540