This is an archive of the discontinued LLVM Phabricator instance.

Derive builtin return type from its definition
ClosedPublic

Authored by mantognini on Oct 4 2018, 3:54 AM.

Details

Summary

Prior to this patch, OpenCL code such as the following would attempt to create
a BranchInst with a non-bool argument:

if (enqueue_kernel(get_default_queue(), 0, nd, ^(void){})) /* ... */

This patch is a follow up on a similar issue with pipe builtin
operations. See commit r280800 and https://bugs.llvm.org/show_bug.cgi?id=30219.

This change, while being conservative on non-builtin functions,
should set the type of expressions invoking builtins to the
proper type, instead of defaulting to bool and requiring
manual overrides in Sema::CheckBuiltinFunctionCall.

In addition to tests for enqueue_kernel, the tests are extended to
check other OpenCL builtins.

Diff Detail

Event Timeline

mantognini created this revision.Oct 4 2018, 3:54 AM

LGTM from OpenCL side!

Anastasia accepted this revision.Nov 23 2018, 6:46 AM

LGTM! It looks reasonable small change!

This revision is now accepted and ready to land.Nov 23 2018, 6:46 AM
This revision was automatically updated to reflect the committed changes.

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

Mainly because I was focusing on OpenCL builtins and was confident about changing those, but wanted to be conservative on code that I don't test/work with. If one wanted to remove those, I would encourage them to double check their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) some for OpenCL.

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

Mainly because I was focusing on OpenCL builtins and was confident about changing those, but wanted to be conservative on code that I don't test/work with. If one wanted to remove those, I would encourage them to double check their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) some for OpenCL.

Got it. Thanks for explanation. Perhaps you could leave a TODO here with a note including what you just said so that
we don't forget about it in the future (including the advice to double check the definition) ?

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

@mantognini Ping ? I agree with you that it would be best to derive the return type of builtins from
their definition, but as such I think that this change is incorrect for the reason given above.

What I would like to propose is to revert the changes to the code in Sema, but keep the tests.

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (getNonReferenceType().withoutLocalFastQualifiers())

Note: I cannot revert this and keep the test because the new ones will fail.

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (getNonReferenceType().withoutLocalFastQualifiers())

Note: I cannot revert this and keep the test because the new ones will fail.

It's not that simple unfortunately. The type passed to CallExpr is the type of the call expression,
which is different from the return type. However you can just use FunctionType::getCallResultType.

lib/Sema/SemaExpr.cpp
5518

Please rename this to something more appropriate,
like ResultTy or similar.

5521–5525

Please leave a FIXME indicating that someone should
go through each of the builtins and remove the
various setType when appropriate.

5522

Don't use auto here.

5524

const auto * or just spell the type.

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

That's fine. Thanks for your contribution !

Please see https://reviews.llvm.org/D55136 for the patch addressing these issues.