Page MenuHomePhabricator

[CodeGen][ObjC] Fix assert on calling `__builtin_constant_p` with ObjC objects.
ClosedPublic

Authored by vsapsai on Jan 29 2019, 5:03 PM.

Details

Summary

When we are calling __builtin_constant_p with ObjC objects of
different classes, we hit the assertion

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file include/llvm/Support/Casting.h, line 254.

It happens because LLVM types for ObjCInterfaceType are opaque and
have no name (see CodeGenTypes::ConvertType). As the result, for
different ObjC classes we have different is_constant intrinsics with
the same name llvm.is.constant.p0s_s. When we try to reuse an
intrinsic with the same name, we fail because of type mismatch.

Fix by bitcasting ObjCObjectPointerType to id prior to passing as an
argument to __builtin_constant_p. This results in using intrinsic
llvm.is.constant.p0i8 and correct types.

rdar://problem/47499250

Event Timeline

vsapsai created this revision.Jan 29 2019, 5:03 PM
void accepted this revision.Jan 29 2019, 5:21 PM

LGTM, but may want to wait for other reviewers.

This revision is now accepted and ready to land.Jan 29 2019, 5:21 PM

LGTM, but may want to wait for other reviewers.

Thanks for the prompt review, Bill. I'll wait for other reviewers to confirm the change makes sense for Objective-C.

LGTM with one minor nit.

clang/test/CodeGenObjC/builtin-constant-p.m
2 ↗(On Diff #184334)

Since this is just testing front-end's IRGen, can you pass -disable-llvm-passes ?

vsapsai updated this revision to Diff 184334.Jan 30 2019, 10:59 AM
  • Add in the test -disable-llvm-passes and relax FileCheck expectations to accept alloca-store-load.
hans added a comment.Feb 6 2019, 2:23 AM

What's the status of this patch? Is it something we should merge onto the 8.0 release branch?

@ahatanak, can you please take a look again?

What's the status of this patch? Is it something we should merge onto the 8.0 release branch?

Waiting on a one more review iteration from @ahatanak and hope that afterwards the patch should be good to go.

As for suitability for 8.0 release branch, I would ask @rjmccall for his opinion. As for me, I'm not entirely comfortable with CodeGen changes late in the release cycle. Also looks like in practice it shouldn't be a serious problem, __builtin_constant_p can be optimized out. For example, see https://godbolt.org/z/e1b4dB

ahatanak accepted this revision.Feb 8 2019, 2:32 PM

LGTM

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 3:02 PM
hans added a comment.Feb 12 2019, 2:29 AM

Also looks like in practice it shouldn't be a serious problem, __builtin_constant_p can be optimized out. For example, see https://godbolt.org/z/e1b4dB

Okay, I'm not going to worry about it then. Thanks.