Anastasia added a subscriber: arsenm.
Anastasia added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+ "|%diff{casting $ to type $|casting between types}0,1}2"
+ " changes address space of nested pointer">;
def err_typecheck_incompatible_ownership : Error<
----------------
ebevhan wrote:
> Anastasia wrote:
> > I am wondering if we could just unify with the diagnostic above?
> >
> > We could add another select at the end:
> > " changes address space of %select{nested|}3 pointer"
> That is doable, but all of the 'typecheck' errors/warnings are made to be
> regular. If I add another parameter, there needs to be a special case in
> DiagnoseAssignmentResult for that error in particular.
Oh I see... might not worth it?
================
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int
*arbitary_numbered_ptr, float src) {
- volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr,
src, 0, 0, false);
-}
----------------
ebevhan wrote:
> Anastasia wrote:
> > Does this not compile any more?
> No, these tests were a bit shaky. I'm not even sure what they're supposed to
> be testing. It's trying to pass an arbitrary `AS int *` pointer to a function
> that takes `__local float *`. That AS conversion is illegal (implicitly), but
> the illegal conversion was 'shadowed' by the 'incompatible pointer' warning,
> so we didn't get an error. This is one of the things this patch fixes.
>
> Since it's a codegen test, it should be producing something, but I'm not
> really sure what is interesting to produce here, so I just removed it.
Ok, I will just loop in @arsenm to confirm. OpenCL doesn't regulate arbitrary
address spaces. And C doesn't regulate OpenCL ones. So interplay between those
two has undefined behavior in my opinion. However, OpenCL code can make use of
arbitrary address spaces since it's a valid Clang extension... But I am not
sure what happens with this undefined behaviors.
For this specific case I would rather expect an error... but not sure it's
worth testing this anyway.
May be Matt can provide us some more insights!
================
Comment at: test/SemaOpenCL/address-spaces.cl:89
+ __local int * __global * __private * lll;
+ lll = gg; // expected-warning {{incompatible pointer types assigning to
'__local int *__global **' from '__global int **'}}
+}
----------------
ebevhan wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > This doesn't seem entirely correct still, but I'm not sure what to do
> > > > about it.
> > > Is it because `Sema::IncompatiblePointer` has priority? We might want to
> > > change that. I think it was ok before because qualifier's mismatch was
> > > only a warning but now with the address spaces we are giving an error. I
> > > wonder if adding a separate enum item for address spaces (something like
> > > `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> > > Is it because `Sema::IncompatiblePointer` has priority?
> >
> > Sort of. The problem is that the AS pointee qualifiers match up until the
> > 'end' of the RHS pointer chain (LHS: `private->global->local`, RHS:
> > `private->global`), so we never get an 'incompatible address space' to
> > begin with. We only get that if 1) the bottommost type is equal after
> > unwrapping pointers (as far as both sides go), or 2) any of the 'shared' AS
> > qualifiers (as far as both sides go) were different.
> >
> > The idea is that stopping when either side is no longer a pointer will
> > produce 'incompatible pointers' when you have different pointer depths, but
> > it doesn't consider anything below the 'shallowest' side of the pointer
> > chain, so we miss out on any AS mismatches further down.
> >
> > (Not that there's anything to mismatch, really. There is no matching
> > pointer on the other side, so what is really the error?)
> >
> > What should the criteria be for when the pointer types 'run out'? I could
> > have it keep digging through the other pointer until it hits a different
> > AS? This would mean that this:
> > ```
> > int **** a;
> > int ** b = a;
> > ```
> > could give a different warning than it does today, though (incompatible
> > nested qualifiers instead of incompatible pointers, which doesn't make
> > sense...) . We would have to skip the `lhptee == rhptee` check if we 'kept
> > going' despite one side not being a pointer type. So I don't know if that's
> > the right approach in general.
> >
> > Or should we be searching 'backwards' instead, starting from the innermost
> > pointee? I don't know.
> >
> > It really feels like the whole `checkPointerTypesForAssignment` routine and
> > everything surrounding it is a bit messy. It relies on an implicit result
> > from another function (`typesAreCompatible`) and then tries to deduce why
> > that function thought the types weren't compatible. Then another function
> > later on (`DiagnoseAssignmentResult`) tries to deduce why THIS function
> > thought something was wrong.
> >
> > > I wonder if adding a separate enum item for address spaces (something
> > > like `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> >
> > This would simplify the logic on the error emission side, since we don't
> > need to duplicate the logic for determining what went wrong, but doesn't
> > help with diagnosing the actual problem. Probably a good idea to add it
> > anyway, I just wanted to avoid adding a new enum member since that means
> > updating a lot of code elsewhere.
> > We only get that if 1) the bottommost type is equal after unwrapping
> > pointers (as far as both sides go), or 2) any of the 'shared' AS qualifiers
> > (as far as both sides go) were different.
>
> Sorry, should only be 2) here. Was focused on the whole 'incompatible nested
> qualifiers' result.
> What should the criteria be for when the pointer types 'run out'? I could
> have it keep digging through the other pointer until it hits a different AS?
Hmm, good point! C99 spec seems to be helpless. C++ seems to imply that it
checks pointers left to right as far as I interpret that from [conv.qual]. Not
sure what we should do... Would it make sense to align with C++ or otherwise
whatever is simpler? At least there is a diagnostic generated. So perhaps
after all it's good enough for now!
> I wonder if adding a separate enum item for address spaces (something
> like Sema::IncompatibleNestedPointerAddressSpace) would simplify things.
>
> This would simplify the logic on the error emission side, since we don't need
> to duplicate the logic for determining what went wrong, but doesn't help with
> diagnosing the actual problem. Probably a good idea to add it anyway, I just
> wanted to avoid adding a new enum member since that means updating a lot of
> code elsewhere.
Ok, common helper function could be another solution to avoid duplication but
it seems the logic is not entirely identical.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58236/new/
https://reviews.llvm.org/D58236
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
