Page MenuHomePhabricator

Changes for LLVM PPCISelLowering function combineBVOfConsecutiveLoads
AbandonedPublic

Authored by sarveshtamba on Apr 11 2019, 4:44 AM.

Details

Reviewers
None
Summary

Seeing the following issue while building swift tool chain on ppc64le:-

swift: /home/sar/swift-source/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:12111: llvm::SDValue combineBVOfConsecutiveLoads(llvm::SDNode *, llvm::SelectionDAG &): Assertion `!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) && "The loads cannot be both consecutive and reverse consecutive."' failed.

More details in https://bugs.llvm.org/show_bug.cgi?id=41177

Making the changes based on the below suggestion which fixes the issue and the tool chain builds successfully:-

Hi, Sarvesh,
I think there is a bug in below code.

for (int i = 1, e = N->getNumOperands(); i < e; ++i) {
  // If any inputs are fp_round(extload), they all must be.
  if (IsRoundOfExtLoad && N->getOperand(i).getOpcode() != ISD::FP_ROUND)
    return SDValue();
  SDValue NextInput = IsRoundOfExtLoad ? N->getOperand(i).getOperand(0) :
    N->getOperand(i);
  if (NextInput.getOpcode() != ISD::LOAD)
    return SDValue();
  SDValue PreviousInput =
    IsRoundOfExtLoad ? N->getOperand(i-1).getOperand(0) : N->getOperand(i-1);
  LoadSDNode *LD1 = dyn_cast<LoadSDNode>(PreviousInput);
  LoadSDNode *LD2 = dyn_cast<LoadSDNode>(NextInput);
  // If any inputs are fp_round(extload), they all must be.
  if (IsRoundOfExtLoad && LD2->getExtensionType() != ISD::EXTLOAD)
    return SDValue();
  if (!isConsecutiveLS(LD2, LD1, ElemSize, 1, DAG))
    InputsAreConsecutiveLoads = false;
  if (!isConsecutiveLS(LD1, LD2, ElemSize, 1, DAG))
    InputsAreReverseConsecutive = false;
  // Exit early if the loads are neither consecutive nor reverse consecutive.
  if (!InputsAreConsecutiveLoads && !InputsAreReverseConsecutive)
    return SDValue();
}
assert(!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) &&
       "The loads cannot be both consecutive and reverse consecutive.");

For below if statement, if InputsAreConsecutiveLoads = 0 and InputsAreReverseConsecutive = 0, it will return SDValue().
if (!InputsAreConsecutiveLoads && !InputsAreReverseConsecutive)

return SDValue();

For below 3 situations, if-statement will not return SDValue():
InputsAreConsecutiveLoads = 0 and InputsAreReverseConsecutive = 1,
InputsAreConsecutiveLoads = 1 and InputsAreReverseConsecutive = 0,
InputsAreConsecutiveLoads = 1 and InputsAreReverseConsecutive = 1,

For below assert-statement:
assert(!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) &&
"The loads cannot be both consecutive and reverse consecutive.");

If InputsAreConsecutiveLoads = 1 and InputsAreReverseConsecutive = 1,
this assert condtion '!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive)' will get false.

So I think the right assert statement should be:
assert((InputsAreConsecutiveLoads || InputsAreReverseConsecutive) &&
"The loads cannot be both consecutive and reverse consecutive.");

Thanks,
Zhang Kang (张抗)
XL Compiler Backend(TOBEY) developer, CDL Shanghai

Email: shkzhang@cn.ibm.com Phone: 86-21-60928940

关注IBM中国编译器开发团队 - 新浪微博: http://www.weibo.com/ibmcompiler | developerWorks: http://ibm.co/HK0GCx

Diff Detail

Repository
rL LLVM

Event Timeline

sarveshtamba created this revision.Apr 11 2019, 4:44 AM
hfinkel added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
12110

You put || in the description, but this still says &&. Did you mean ||?

12111

This text needs to be updated too?

jsji added a subscriber: ZhangKang.Apr 11 2019, 7:59 AM

@ZhangKang Looks like the patch is suggested by you? Do you have any testcases that can validate this?
And also are you able to reproduce (and narrow down) the problem before suggesting the patch?

@jsji , I will talk with @sarveshtamba to check and add the test case.

Hi @nemanjai,
I saw that this part of the code was introduced by you in commit - https://reviews.llvm.org/D26023
Can you please tell what this piece of code is doing and if at all the assert statement needs to be revisited?

@ZhangKang and me are trying to fix this issue on ppc64le.

Thanks in advance!

This path has been abandoned, the new patch for this bug is in https://reviews.llvm.org/D60811.

sarveshtamba abandoned this revision.Apr 16 2019, 9:55 PM

The new patch for this bug is in https://reviews.llvm.org/D60811.