Page MenuHomePhabricator

[PowerPC] PPCBoolRetToInt: Don't translate Constant's operands
ClosedPublic

Authored by lkail on Jul 31 2020, 12:49 AM.

Details

Summary

When collecting i1 values via findAllDefs, ignore Constant's operands, since Constant's operands might not be i1.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46923 which causes ICE

llvm-project/llvm/lib/IR/Constants.cpp:1924: static llvm::Constant *llvm::ConstantExpr::getZExt(llvm::Constant *, llvm::Type *, bool): Assertion `C->getType()->getScalarSizeInBits() < Ty->getScalarSizeInBits()&& "SrcTy must be smaller than DestTy for ZExt!"' failed.

Diff Detail

Event Timeline

lkail created this revision.Jul 31 2020, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 12:49 AM
lkail requested review of this revision.Jul 31 2020, 12:49 AM
lkail edited the summary of this revision. (Show Details)Jul 31 2020, 12:57 AM
lkail edited the summary of this revision. (Show Details)
lkail added a subscriber: hans.Aug 6 2020, 4:39 AM

I can confirm that this patch also fixes the full test case.

lkail added a comment.Aug 7 2020, 7:43 PM

Thanks Tom for testing it. Gentle ping reviewers.

hans added a comment.Aug 18 2020, 6:34 AM

Carrot, Kit: I think you're most familiar with this code. Ping?

I think your test case is a perfect candidate for this optimization. It should not be skipped.

The real problem is in function findAllDefs. It should not find out the operands of Constant and push them into Defs.
Suppose we have following ConstantExpr C0 as a phi operand

C0 = C1 > C2

C0 is i1 type, C1 and C2 can be i64 type. And later we should call function translate on C0 only. Translate the type of C1 and C2 doesn't make sense.

lkail updated this revision to Diff 287575.EditedTue, Aug 25, 12:02 AM
lkail retitled this revision from [PowerPC] PPCBoolRetToInt: Skip translation if there is ConstantExpr to [PowerPC] PPCBoolRetToInt: Don't translate Constant's operands.

@Carrot Thanks for your awesome solution.

Please also update your patch description. Otherwise it looks good to me.

lkail edited the summary of this revision. (Show Details)Wed, Aug 26, 7:00 PM
lkail added a comment.Wed, Aug 26, 7:03 PM

Updated Summary.

Carrot accepted this revision.Thu, Aug 27, 8:51 AM
This revision is now accepted and ready to land.Thu, Aug 27, 8:51 AM
hans added a comment.Fri, Aug 28, 2:01 AM

Pushed to 11.x as b931e22c954374acf75c4f1d1f2666f3f8e67470. Please let me know if there are any follow-ups.