This is an archive of the discontinued LLVM Phabricator instance.

Fix a mergefunc crash caused by bitcasting intrinsics
ClosedPublic

Authored by dotdash on Apr 21 2014, 5:57 AM.

Details

Reviewers
dyatkovskiy

Diff Detail

Event Timeline

dotdash updated this revision to Unknown Object (????).Apr 22 2014, 2:07 AM

Generally skip the bitcast when dealing with function constants

Bitcasting function constants is rather pointless here, as bitcasting one will
not result in the other. So we can return early whenever one of the constants
is a function, instead of having a special case for intrinsics.

silvas added a subscriber: silvas.

Stepan could you take a look at this?

dyatkovskiy edited edge metadata.Apr 22 2014, 10:52 PM

Dotdash, thank you for working on it!

For merge functions side this check looks right to me.
So, once you realized V1 != V2, and:

  • One of them is Function.
  • Even if both of V1 and V2 are functions, actually, we have no proper way to detect whether those are definitions of the same function.

Well, IMHO, its OK, to return false.

I suspect this code:

C1 == ConstantExpr::getBitCast(const_cast<Constant*>(C2), C1->getType());

We didn't expect it will return "true" for such bitcast (from "i8* @breaker(i8*)" to "i64 @llvm.bswap.i64(i64)". Those are different functions. Perhaps *this* is the real bug?

I'm not sure how it could be done. May be like this:

  1. File new bug for getBitCast method.
  2. I'd propose to cut off MergeFunctions dependency from #1, and commit Dotdash's patch. I also suspect it could take a really long time to localize #1, then fix it, and then get LGTM.

2014-04-23 7:52 GMT+02:00 Stepan Dyatkovskiy <stpworld@narod.ru>:

I suspect this code:

C1 == ConstantExpr::getBitCast(const_cast<Constant*>(C2), C1->getType());

We didn't expect it will return "true" for such bitcast (from "i8* @breaker(i8*)" to "i64 @llvm.bswap.i64(i64)". Those are different functions. Perhaps *this* is the real bug?

It doesn't return true. The bitcast is never actually emitted (I
checked the resulting IR when running with -disable-verify) and the
functions aren't merged. But AFAICT the bitcast is added to the list
of users of the intrinsic and that's enough for the verifier to
complain about it.

Another approach I initially had in mind was to modify
canLosslesslyBitCastTo to return false for intrinsics. Yet another
solution would be to destroy the bitcast instruction after the
comparison.

dyatkovskiy added inline comments.Apr 22 2014, 11:12 PM
test/Transforms/MergeFunc/intrinsics.ll
2

I'd propose to check "-stats" output here as well.

12

Instead of -disable-output, I'd propose to direct it to FileCheck and ensure both of @collider and @bswap_caller appear in output in their original state.
As example you could use:

  1. test/Transforms/MergeFunc/too-small.ll
  2. test/Transforms/MergeFunc/ptr-int-transitivity-1.ll

Though, instead of 'not grep "functions merged"'
you could just use CHECK-NOT in the end of test.

dyatkovskiy added a comment.EditedApr 22 2014, 11:22 PM

Björn,
I think the fix of canLosslesslyBitcasted could be only accepted in case, when functions are not bitcastable at all. But I think they are. At least you can define such bitcast in globals. So I think, we better do our best from MergeFunctions side, keep it stable, file a bug, so then we could work on it afterwards.

dyatkovskiy added a comment.EditedApr 22 2014, 11:25 PM

It doesn't return true.

But then, how FunctionComparator::enumerate could return "true" in this case?
Result of enumerate method is based on conditions "can-be-bitcasted" && "C1 == bitcast of C2". Have I missed something?

Stepan,
do you still want the test to be adjusted, given that the functions aren't actually merged? If so, I'd have to increase the size of both functions, for it to make any sense, because they're currently too small to be merged anyway. Doing so would be ok with me, but doesn't seem relevant to the bug at hand.

Regarding canLosslesslyBitCastTo, I would have checked for isIntrinsic() there. But yeah, let's keep to to mergefunc for now.

dyatkovskiy added a comment.EditedApr 22 2014, 11:33 PM

Yes, it would be good if you increase test size. So we could be sure it will run whole the comparison routines.

It doesn't return true.

But then, how FunctionComparator::enumerate could return "true" in this case?

Sorry, for that question, but it looks like crash happens somewhere inside getBitcast, right?

I probably shouldn't have said crash. The mergefunc pass completes. Without merging the functions. But in enumerate, it created that bitcast of the intrinsic, which is added to the list of users of that intrinsic. What happens then is that the verify pass checks all users of all intrinsics, sees the bitcast, complains about it and aborts.

dyatkovskiy added a comment.EditedApr 22 2014, 11:55 PM

So, getBitCast creates new Value (call it C3).
C3 is not equal to C1, but it still hangs in air.
Perhaps then just as another solution, we could do the next:

  1. Check for bitcast possibility
  2. Create bitcast (C3)
  3. Check whether C1 is equal to C3
  4. Get rid of C3.
dyatkovskiy added a comment.EditedApr 23 2014, 12:58 AM

I have talked with Chandler in IRC. My last suggestion contradicts LLVM design, because constants should never been removed.
So, that means we better stay on your current solution. I would be glad if you fix the tests anyway ;-)
Thanks!

Yeah, that's what I meant earlier. The result would look like this:

if (!C1->getType()->canLosslesslyBitCastTo(C2->getType()))
  return false;
Constant *C3 = ConstantExpr::getBitCast(const_cast<Constant*>(C2), C1->getType());
bool result = C1 == C3;
if (C3->use_empty())
  C3->destroyConstant();
return result;

But I wonder if we really want that. Compared to the other patch, this means that we still create pointless bitcasts for functions, i.e. it's more expensive. And even if we apply both patches, that would mean that we would possibly create and destroy the same constant over and over again, each time the same V2 function is passed to enumerate again.

Which version do you prefer?

dotdash updated this revision to Diff 8760.Apr 23 2014, 1:15 AM
dotdash edited edge metadata.

Fix a mergefunc crash caused by bitcasting intrinsics

In general, if we're dealing with two distinct function constants, bitcasting
one of them will not result in the other, so we can skip the cast and just
return false.

dotdash updated this revision to Diff 8761.Apr 23 2014, 1:19 AM

Fixed the test to include -stats

Sorry, didn't notice that as there's no way to actually make that CHECK-NOT
assetion fail with the given test case.

dyatkovskiy accepted this revision.Apr 23 2014, 1:24 AM
dyatkovskiy edited edge metadata.

Its good to me! Sean, what do you think?

Thanks for working on it!

P.S.: Please ensure next buildbot will stay green after your commit:
http://lab.llvm.org:8011/builders/clang-mergefunc-x86_64-freeBSD9.2

This revision is now accepted and ready to land.Apr 23 2014, 1:24 AM
dotdash updated this revision to Diff 8762.Apr 23 2014, 1:27 AM
dotdash edited edge metadata.

Really fix the -stats related part of the test

-stats goes to stderr, so FileCheck didn't see that.

Hi Sean, Hi Nick,

could one of you have another look at this? Stepan accepted this but told me on IRC that one of you has to give a final LGTM and commit it.

Thanks!

Hi Bjorn,
We have replaced "enumerate" method with "cmpValues" + "cmpConstants". That was made as part of bigger improvement.
Now getBitcast is not called. But your test is really usefull.
So I would like to ask you try your test with trunk llvm version. I think it should work now. If so, I try to organize commit for you today.

P.S.: Do you have write access?

dotdash updated this revision to Diff 10158.Jun 5 2014, 3:02 PM

Removed the now obsolete changed to mergefunc, keeping just the regression tests

Hi Stepan,

thanks, I had completely forgot to update the patch before asking for another review. I removed the changes to mergefunc itself and only kept test, which passes just fine. And while I didn't get to test it myself yet, according to a rust developer, rustc also compiles with the mergefunc pass enabled (that this failed was the original reason for the patch).

No, I do not have write access.

Thanks
Björn

This comment was removed by dyatkovskiy.

OK. I'll try to commit it for you in nearest days. All I need is meet Nick in IRC and ask about this test.

dyatkovskiy closed this revision.Jun 9 2014, 5:23 PM

This test has been committed with some changes as r210486. Thanks!