This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix wrong ElemSIze when calling isConsecutiveLS()
ClosedPublic

Authored by ZhangKang on Apr 16 2019, 9:46 PM.

Details

Summary

This issue from the bugzilla: https://bugs.llvm.org/show_bug.cgi?id=41177

When the two operands for BUILD_VECTOR are same, we will get assert error.

llvm::SDValue combineBVOfConsecutiveLoads(llvm::SDNode*, llvm::SelectionDAG&): Assertion `!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) && "The loads cannot be both consecutive and reverse consecutive."' failed.

This error caused by the wrong ElemSIze when calling isConsecutiveLS(). We should use getScalarType().getStoreSize(); to get the ElemSize instread of getScalarSizeInBits() / 8.

Diff Detail

Event Timeline

ZhangKang created this revision.Apr 16 2019, 9:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 9:46 PM
jsji accepted this revision.Apr 17 2019, 8:39 PM

LGTM. Thanks for reducing the testcase and fixing!

This revision is now accepted and ready to land.Apr 17 2019, 8:39 PM
This revision was automatically updated to reflect the committed changes.

This fix is wrong. For vectors where the element type is not byte-sized, loads can't be "consecutive" in the sense of this function; the store size of <2 x i1> is one byte.

The right fix is to return early from combineBVOfConsecutiveLoads for vector types where getVectorElementType().isByteSized() is false. Or you could probably just check !isTypeLegal(N->getValueType(0)); even if the transform is legal, it likely isn't profitable for illegal vector types.

jsji added a comment.Apr 18 2019, 3:18 PM

For vectors where the element type is not byte-sized, loads can't be "consecutive" in the sense of this function; the store size of <2 x i1> is one byte.

I think isConcecitiveLS will early return false too with this fix. getScalarSizeInBits() / 8 != Bytes.

I guess that's right, technically; "getScalarSizeInBits() / 8" will round down, and getStoreSize() will round up. But still, please fix the code so it's clear that check is happening.

jsji added a comment.Apr 18 2019, 3:42 PM

please fix the code so it's clear that check is happening.

Sure, we will follow up with a NFC patch with your suggestions. Thanks for pointing out.

I guess that's right, technically; "getScalarSizeInBits() / 8" will round down, and getStoreSize() will round up. But still, please fix the code so it's clear that check is happening.

I have submitted a NFC patch D61076 to follow your suggestion.