Fix a mergefunc crash caused by bitcasting intrinsics

[dotdash]

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]

Stepan could you take a look at this?

[dyatkovskiy]

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.

[dotdash]

2014-04-23 7:52 GMT+02:00 Stepan Dyatkovskiy <[e-mail]>:

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]

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]

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]

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?

[dotdash]

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]

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?

[dotdash]

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]

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]

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!

[dotdash]

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]

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]

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]

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

[dotdash]

Really fix the -stats related part of the test

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

[dotdash]

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!

[dyatkovskiy]

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]

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

[dotdash]

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

[dyatkovskiy]

This comment was removed by dyatkovskiy.

[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]

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

lib/Transforms/IPO/MergeFunctions.cpp

bool FunctionComparator::enumerate(const Value *V1, const Value *V2) {
   if (const Constant *C1 = dyn_cast<Constant>(V1)) {
     if (V1 == V2) return true;
     const Constant *C2 = dyn_cast<Constant>(V2);
     if (!C2) return false;
     // TODO: constant expressions with GEP or references to F1 or F2.
     if (C1->isNullValue() && C2->isNullValue() &&
         isEquivalentType(C1->getType(), C2->getType()))
       return true;
+    if (isa<Function>(V1) || isa<Function>(V2))
+      return false;
     // Try bitcasting C2 to C1's type. If the bitcast is legal and returns C1
     // then they must have equal bit patterns.
     return C1->getType()->canLosslesslyBitCastTo(C2->getType()) &&
       C1 == ConstantExpr::getBitCast(const_cast<Constant*>(C2), C1->getType());
   }
 
   if (isa<InlineAsm>(V1) || isa<InlineAsm>(V2))
     return V1 == V2;

test/Transforms/MergeFunc/intrinsics.ll

+; RUN: opt -S -mergefunc -stats < %s 2>&1 | FileCheck %s
+; This used to crash because mergefunc created a bitcast of an intrinsic.
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i64 @llvm.bswap.i64(i64) unnamed_addr nounwind readnone
+declare i8* @breaker(i8*) unnamed_addr
+
+; @bswap_caller and @collider have the same hash in mergefunc and get fed to
+; enumerate, which used to create a bitcast of the bswap intrinsic that
+; survived on the intrinsics uselist and made the verify pass complain.
+; The repeated calls are just to make the functions large enough to be merged
+; and test that they aren't merged, regardless of the above bug.
+define void @bswap_caller(i64) unnamed_addr {
+; CHECK-LABEL: @bswap_caller(
+entry-block:
+  call i64 @llvm.bswap.i64(i64 %0)
+  call i64 @llvm.bswap.i64(i64 %0)
+  call i64 @llvm.bswap.i64(i64 %0)
+  call i64 @llvm.bswap.i64(i64 %0)
+  ret void
+}
+
+define internal void @collider(i8*) unnamed_addr {
+; CHECK-LABEL: @collider(
+entry-block:
+  call i8* @breaker(i8* %0)
+  call i8* @breaker(i8* %0)
+  call i8* @breaker(i8* %0)
+  call i8* @breaker(i8* %0)
+  ret void
+}
+
+; CHECK-NOT: functions merged