This is an archive of the discontinued LLVM Phabricator instance.

[PR42707][OpenCL] Fix addr space deduction for auto
ClosedPublic

Authored by Anastasia on Aug 5 2019, 6:32 AM.

Details

Summary

A regression was introduced in D64400 because auto is using the same logic as templates. However, deduction of addr spaces wasn't working correctly even before.

Here are the rules that are implemented in this patch:

  • For non-reference and non-pointer types deduction can be done early because addr space is not going to be taken from init expr
  • For ref or ptr auto types we should prevent deducing addr space before the deduction of the whole type (incl its pointee addr spaces) from init expr

Diff Detail

Event Timeline

Anastasia created this revision.Aug 5 2019, 6:32 AM

Isn't the general rule for template argument deduction (which this devolves to) just to ignore top-level qualifiers? And then you can substitute in the substituted type and end up with a properly qualified type for the parameter / variable, and you can add extra qualifiers as necessary. Why are special rules for pointers and references required?

Isn't the general rule for template argument deduction (which this devolves to) just to ignore top-level qualifiers? And then you can substitute in the substituted type and end up with a properly qualified type for the parameter / variable, and you can add extra qualifiers as necessary. Why are special rules for pointers and references required?

The issue I am trying to solve is that the addr space qualifier for the pointee type in pointers and references is supposed to default to generic if none were provided either in the parameter or argument of template/auto type. But for all regular types the deduction happens early during parsing. So I need to prevent the early deduction and make sure the deduction happens once the type is being known (i.e. deduced).

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee? I still don't understand why pointers and references are handled differently here, instead of having the rule be "don't infer if the type is opaquely dependent or an auto placeholder".

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee?

It is recursive indeed and we currently deduce all pointees to generic AS.

I still don't understand why pointers and references are handled differently here, instead of having the rule be "don't infer if the type is opaquely dependent or an auto placeholder".

Because we currently only infer pointers and references in TreeTransforms. Everything else is inferred earlier. That is mainly because we need some context info to infer in most of other cases and therefore it's not easy to do in TreeTransforms. There is some more information on this review: https://reviews.llvm.org/D64400 where the inference for pointers and references was added originally to TreeTransforms.

I think this looks good. Maybe the tests should be extended to test auto as function return type, and if there's some special handling around decltype(auto), then it should be tested too, but I'm not sure it's actually needed here. What do you think?

lib/Sema/SemaType.cpp
7441 ↗(On Diff #213345)

Shouldn't the parentheses around IsAutoPointee be removed for style consistency?

7441 ↗(On Diff #213345)

With the if statement introduced above, IsAutoPointee can be true only in C++ mode. Could it be an issue to not guard (T->isAutoType() && IsPointee) for non-C++ mode? (I guess not, but I couldn't convince myself.)

lib/Sema/TreeTransform.h
4550 ↗(On Diff #213345)

Nitpicking: there are two spaces between } and while.

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee?

It is recursive indeed and we currently deduce all pointees to generic AS.

Is that likely to change? If all pointees are inferred to be generic (in the absence of an explicit qualifier) when building a pointer type, this seems very over-thought vs. just adding the generic AS whenever you build a pointer to an unqualified but non-dependent type.

Anastasia marked an inline comment as done.Aug 30 2019, 3:24 AM

missing test case

1 auto* accumulator()
2 {
3   __global int * glptr;
4   return glptr;
5 }
6 int i;
7 auto* ai = &i;
8 int* ii = &i;

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee?

It is recursive indeed and we currently deduce all pointees to generic AS.

Is that likely to change? If all pointees are inferred to be generic (in the absence of an explicit qualifier) when building a pointer type, this seems very over-thought vs. just adding the generic AS whenever you build a pointer to an unqualified but non-dependent type.

I don't think this is likely to change. Are you suggesting to move the deduction logic for pointee of pointers, references and block pointers into ASTContext helper that creates a pointer/reference/block pointer type? I guess it should work if the only way to construct the pointer is by using this helper. I can certainly prototype this change and see.

lib/Sema/SemaType.cpp
7441 ↗(On Diff #213345)

I think TreeTransforms will only be used in C++ mode. But also isAutoType should only be true in C++ mode. So I think we should be fine.

I don't think this is likely to change. Are you suggesting to move the deduction logic for pointee of pointers, references and block pointers into ASTContext helper that creates a pointer/reference/block pointer type?

No. I'm suggesting that the deduction logic should be much more straightforward, just some sort of "is the type non-dependent and lacking a qualifier", and it should be applied in the two basic places we build these types in Sema, i.e. in the type-from-declarator logic and in the Build{Pointer,Reference}Type logic. Instead we have something very elaborate that apparently recursively looks through pointer types and is contingent on the exact spelling, e.g. trying to find auto types, which seems both brittle and unnecessary.

include/clang/AST/Type.h
6509 ↗(On Diff #213345)

Hmm. So this method, confusingly, will not return true for a deduced auto, unless the deduced type is itself an undeduced auto (which I'm not sure can happen). I think it at least needs a different name; isUndeducedAutoType() would be okay if the latter case is not possible. But it might be better if we can just define away the need for the method entirely.

lib/Sema/SemaType.cpp
7441 ↗(On Diff #213345)

I don't think TreeTransform is expected to be C++-only, but I agree that isAutoType is good enough.

Anastasia updated this revision to Diff 218639.Sep 4 2019, 4:01 AM

Moved addr space of pointee inference into Build* helpers of Sema.

I don't think this is likely to change. Are you suggesting to move the deduction logic for pointee of pointers, references and block pointers into ASTContext helper that creates a pointer/reference/block pointer type?

No. I'm suggesting that the deduction logic should be much more straightforward, just some sort of "is the type non-dependent and lacking a qualifier", and it should be applied in the two basic places we build these types in Sema, i.e. in the type-from-declarator logic and in the Build{Pointer,Reference}Type logic.

Ok thanks, I made this change now.

Instead we have something very elaborate that apparently recursively looks through pointer types and is contingent on the exact spelling, e.g. trying to find auto types, which seems both brittle and unnecessary.

I realized that I didn't have to do this actually. It was done by mistake earlier.

Anastasia marked an inline comment as done.Sep 4 2019, 4:05 AM
Anastasia added inline comments.
include/clang/AST/Type.h
6509 ↗(On Diff #213345)

I feel I still need this helper in order to detect the auto type. Alternatively I can create a static function for this instead of publicly exposing this functionality. Or maybe you have other ideas in mind...

I think this looks good. Maybe the tests should be extended to test auto as function return type, and if there's some special handling around decltype(auto), then it should be tested too, but I'm not sure it's actually needed here. What do you think?

Do you mean anything specific to test?

Anastasia updated this revision to Diff 218661.Sep 4 2019, 5:47 AM
  • Added forgotten test
rjmccall added inline comments.Sep 4 2019, 1:55 PM
clang/lib/Sema/SemaType.cpp
7396

Okay, I understand why you're doing this now, and it makes sense. I would like to propose changing the entire way deduceOpenCLImplicitAddrSpace works. Why don't we do it more like how ObjC ARC infers its implicit ownership qualifiers:

  • In SemaType, we add the default address space to non-qualified, non-dependent, non-undeduced-auto pointees when parsing a pointer or reference type.
  • In SemaType, we add the default address space to non-qualified pointees when building a pointer or reference type.
  • We add the default address space at the top level when when building a variable.

Then all of this context-specific logic where we're looking at different declarator chunks and trying to infer the relationship of the current chunk to the overall type being parsed can just go away or get pushed into a more appropriate position.

Anastasia updated this revision to Diff 219903.Sep 12 2019, 6:32 AM
  • Move addr space deduction to a later phase.
Anastasia marked 2 inline comments as done.Sep 12 2019, 6:42 AM
Anastasia added inline comments.
clang/lib/Sema/SemaDecl.cpp
6125

I don't seem to need a check for dependent or auto types because the substitution happens using type info rather than getting type from the declaration. Not sure if I should explain it here or add the checks just in case?

clang/lib/Sema/SemaType.cpp
7396

Ok, it mainly works, however I still need a bit of parsing horribleness when deducing addr space of declarations with parenthesis in GetFullTypeForDeclarator. This is the case for block pointers or pointers/references to arrays. It is incorrect to add address spaces on ParenType while building a pointer or references so I have to detect this as special case.

rjmccall added inline comments.Sep 25 2019, 2:36 PM
clang/lib/Sema/SemaDecl.cpp
6125

If you have adequate test-case coverage (both inside and out of templates) then I don't think you need further explanation.

6971

Since you're moving this code anyway, can this be split into its own function? I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.

clang/lib/Sema/SemaType.cpp
7396

You can't add an address space outside a ParenType? That seems odd; what problems are you seeing exactly?

If it's really just specific to ParenType, you could simply drill through them and then rebuild the ParenTypes.

Anastasia marked 2 inline comments as done.Sep 25 2019, 5:53 PM
Anastasia added inline comments.
clang/lib/Sema/SemaDecl.cpp
6971

I'm not sure if it's actually important that some of these failures return immediately and some of them fall through to later checks.

Yes, it looks a bit random. Do we have any guideline whether we should return or keep diagnosing as long as possible?

clang/lib/Sema/SemaType.cpp
7396

You can't add an address space outside a ParenType? That seems odd; what problems are you seeing exactly?

When we add addr space for a pointee and it's a ParenType the addr space should actually be added to a first non-ParenType but not ParenType itself. For example is we declare a reference to an array we want the array type to have an address space not ParenType.

If it's really just specific to ParenType, you could simply drill through them and then rebuild the ParenTypes.

Ok, in the case I explained above we would have to add address space to the first non-ParenTypes and then rebuild all ParenTypes. I will try that.

rjmccall added inline comments.Sep 25 2019, 11:01 PM
clang/lib/Sema/SemaDecl.cpp
6971

If the user is likely to have made multiple independent errors, it's good to diagnose as many of them as possible. But if it's just as likely that the user messed up in a single way that should've meant we didn't take this code path, then it's better to bail out early.

In this case, most of these diagnostics are looking for different special OpenCL types, and those are probably all independent of each other.

...it does occur to me to wonder if more of these checks should be looking through array types the way that the check for half does. Presumably you shouldn't be able to declare global arrays of images or pipes if you can't declare non-array variables of them.

clang/lib/Sema/SemaType.cpp
7396

ParenType is just a sugar node, and qualifiers on it should be treated identically to qualifiers on the inner type, just like a qualifier on a typedef name (i.e. const int32_t) would. So unless you're seeing a real problem I wouldn't worry about it.

I could totally believe that it prints poorly, though.

Anastasia updated this revision to Diff 227841.Nov 5 2019, 3:22 AM
Anastasia marked an inline comment as done.
  • Factored OpenCL diagnostics out in a separate helper function
  • Removed special case for ParenType
clang/lib/Sema/SemaDecl.cpp
6971

...it does occur to me to wonder if more of these checks should be looking through array types the way that the check for half does. Presumably you shouldn't be able to declare global arrays of images or pipes if you can't declare non-array variables of them.

We actually provide dedicated diagnostic for all other types in Sema::BuildArrayType. No idea why half is handled here I will try to refactor that in a separate patch.

rjmccall accepted this revision.Nov 7 2019, 10:19 AM
rjmccall added inline comments.
clang/lib/Sema/SemaDecl.cpp
6971

Well, half is a perfectly reasonable type to have an array of. I don't know why that's not equally true of images or pipes; are they assumed to have implicit trailing storage? Anyway, if OpenCL says arrays of them are forbidden, we don't need to look through arrays; that's probably worth a comment, though.

This revision is now accepted and ready to land.Nov 7 2019, 10:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2019, 4:47 AM