yaxunl added inline comments.
================
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
- OS << T->getEquivalentType().getAddressSpace();
+ OS << T->getEquivalentType()
+ .getQualifiers()
----------------
arichardson wrote:
> yaxunl wrote:
> > arichardson wrote:
> > > arichardson wrote:
> > > > Anastasia wrote:
> > > > > arichardson wrote:
> > > > > > Anastasia wrote:
> > > > > > > arichardson wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > Why do we need this change?
> > > > > > > > `__attribute__((address_space(n)))` is a target address space
> > > > > > > > and not a language address space like `LangAS::opencl_generic`.
> > > > > > > > Isn't `Qualifiers::getAddressSpaceAttributePrintValue()` meant
> > > > > > > > exactly for this use case?
> > > > > > > Yes, I think there are some adjustment we do in this method to
> > > > > > > get the original source value to be printed corerctly. Does this
> > > > > > > mean we have no tests that caught this issue?
> > > > > > Seems like it, all tests pass both with and without this patch.
> > > > > Strange considering that we have this attribute printed in some error
> > > > > messages of some Sema tests. If I compile this code without your
> > > > > patch:
> > > > >
> > > > > ```
> > > > > typedef int __attribute__((address_space(1))) int_1;
> > > > > typedef int __attribute__((address_space(2))) int_2;
> > > > >
> > > > > void f0(int_1 &);
> > > > > void f0(const int_1 &);
> > > > >
> > > > > void test_f0() {
> > > > > int i;
> > > > > static int_2 i2;
> > > > > f0(i);
> > > > > f0(i2);
> > > > > }
> > > > > ```
> > > > >
> > > > > I get the address spaces printed correctly inside the type:
> > > > > note: candidate function not viable: 1st argument ('int_2' (aka
> > > > > '__attribute__((address_space(2))) int')) is in address space 2, but
> > > > > parameter must be in address space 1
> > > > >
> > > > > Perhaps @yaxunl could comment further on whether this change is
> > > > > needed.
> > > > My guess is that it doesn't go through that switch statement but rather
> > > > through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to
> > > > see if there are any tests that go down this path.
> > > I just ran the clang tests with an llvm_unreachable() here and none of
> > > them failed. So it seems like we don't have anything testing this code
> > > path.
> > Sorry for the delay. This part of code is for printing the addr space of
> > AttributedType. Since it seems not used by any language yet, there is no
> > test for it. It is possible a non-target-specific address space being
> > printed here if a language chooses to use AttributedType to represent
> > address space. Therefore a proper fix would be isolate the code for
> > printing address space from Qualifiers::print and re-use it here so that
> > addr space is printed in consistent way no matter it is represented as
> > qualifier or as AttributedType.
> Thanks, that makes sense. To avoid breaking anything here I think it should
> be part of a separate patch though.
Sure. In this one probably keep the original behavior.
https://reviews.llvm.org/D38816
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
