[Clang][TableGen] Support specifying address space in clang builtin prototypes

[vikramRH]

this is a follow up from the discussion in #86801 (apologies for the long delay...).
This PR proposes a way to specify address spaces in builtin prototypes. The idea is to specify address space numbers using CXX11 attribute list syntax ([[<attr_list>]]) with following limitations,

1. The attribute [[addrspace[n]]] is strictly a "prefix" to the builtin type, i.e something as follows is not accepted,
   int* const [[addrspace[3]]] ;
2. I really wanted the syntax to be like [[addrspace(n)]] but '(' token conflicts with function signature. so current approach is to use "[[addrspace[n]]]"
3. The attribute is only valid with pointer and reference types (as per the restriction imposed by .def files)

I would like some views on this approach and alternate suggestions if any. Also please let me know if there are any parallel efforts towards this which I might not be aware of.

[vikramRH]

Gentle ping @AaronBallman , @philnik777 , @fpetrogalli :)

[AaronBallman]

Gentle ping @AaronBallman , @philnik777 , @fpetrogalli :)

Ah, sorry -- because the PR is marked as a Draft, I figured it wasn't ready for review yet.

I think I'd rather this was expressed differently; we already don't put attribute information in the prototype anyway (noexcept as an example), so I'd prefer to continue down that road and put the address space information into the Attributes field. e.g.,

def BuiltinCPUIs : Builtin {
  let Spellings = ["__builtin_cpu_is"];
  let Attributes = [NoThrow, Const, AddressSpace<2>];
  let Prototype = "bool(char const*)";
}

I think that makes it more clean in terms of specifying the attribute, and it also means we can name the address spaces in BuiltinsBase.td if we would like, which is even easier for folks to understand when reading Builtins.td

WDYT?

[vikramRH]

Gentle ping @AaronBallman , @philnik777 , @fpetrogalli :)

Ah, sorry -- because the PR is marked as a Draft, I figured it wasn't ready for review yet.

I think I'd rather this was expressed differently; we already don't put attribute information in the prototype anyway (noexcept as an example), so I'd prefer to continue down that road and put the address space information into the Attributes field. e.g.,

def BuiltinCPUIs : Builtin {
  let Spellings = ["__builtin_cpu_is"];
  let Attributes = [NoThrow, Const, AddressSpace<2>];
  let Prototype = "bool(char const*)";
}

I think that makes it more clean in terms of specifying the attribute, and it also means we can name the address spaces in BuiltinsBase.td if we would like, which is even easier for folks to understand when reading Builtins.td

WDYT?

Thanks for the reply @AaronBallman . The reason this is still a draft is that I wanted it to be an initial proposal to get some inputs and a consensus on the final design. and about it being part of the "Attributes" field, one major issue is that the address space information should be per argument including the return type. "Attributes" field currently expresses attributes to the function. If attribute in the prototype is not desired, probably a new field that lets us specify per argument attributes makes sense ?

[AaronBallman]

Gentle ping @AaronBallman , @philnik777 , @fpetrogalli :)

Ah, sorry -- because the PR is marked as a Draft, I figured it wasn't ready for review yet.
I think I'd rather this was expressed differently; we already don't put attribute information in the prototype anyway (noexcept as an example), so I'd prefer to continue down that road and put the address space information into the Attributes field. e.g.,

def BuiltinCPUIs : Builtin {
  let Spellings = ["__builtin_cpu_is"];
  let Attributes = [NoThrow, Const, AddressSpace<2>];
  let Prototype = "bool(char const*)";
}

I think that makes it more clean in terms of specifying the attribute, and it also means we can name the address spaces in BuiltinsBase.td if we would like, which is even easier for folks to understand when reading Builtins.td
WDYT?

Thanks for the reply @AaronBallman . The reason this is still a draft is that I wanted it to be an initial proposal to get some inputs and a consensus on the final design. and about it being part of the "Attributes" field, one major issue is that the address space information should be per argument including the return type. "Attributes" field currently expresses attributes to the function. If attribute in the prototype is not desired, probably a new field that lets us specify per argument attributes makes sense ?

Oh! I hadn't realized this was needed on a per-parameter basis. Oof that makes this more awkward. I'd still love to avoid writing this as part of the signature; I think we could use the existing IndexedAttribute to specify which argument the attribute applies to. e.g.,

class AddressSpace<int Idx, int AddrSpaceNum> : IndexedAttribute<"something", Idx> {
  int SpaceNum = AddrSpaceNum;
}

[vikramRH]

Gentle ping @AaronBallman , @philnik777 , @fpetrogalli :)

Ah, sorry -- because the PR is marked as a Draft, I figured it wasn't ready for review yet.
I think I'd rather this was expressed differently; we already don't put attribute information in the prototype anyway (noexcept as an example), so I'd prefer to continue down that road and put the address space information into the Attributes field. e.g.,

def BuiltinCPUIs : Builtin {
  let Spellings = ["__builtin_cpu_is"];
  let Attributes = [NoThrow, Const, AddressSpace<2>];
  let Prototype = "bool(char const*)";
}

I think that makes it more clean in terms of specifying the attribute, and it also means we can name the address spaces in BuiltinsBase.td if we would like, which is even easier for folks to understand when reading Builtins.td
WDYT?

Thanks for the reply @AaronBallman . The reason this is still a draft is that I wanted it to be an initial proposal to get some inputs and a consensus on the final design. and about it being part of the "Attributes" field, one major issue is that the address space information should be per argument including the return type. "Attributes" field currently expresses attributes to the function. If attribute in the prototype is not desired, probably a new field that lets us specify per argument attributes makes sense ?

Oh! I hadn't realized this was needed on a per-parameter basis. Oof that makes this more awkward. I'd still love to avoid writing this as part of the signature; I think we could use the existing IndexedAttribute to specify which argument the attribute applies to. e.g.,

class AddressSpace<int Idx, int AddrSpaceNum> : IndexedAttribute<"something", Idx> {
  int SpaceNum = AddrSpaceNum;
}

Makes sense, I will give this a try and update the PR

[vikramRH]

Made some initial changes according to suggestions @AaronBallman , how does this look ?
some additional comments ,

1. The new diagnostics introduced here still point to record definition loc rather than the attribute loc which would be ideal. but I could not find a way to do this
2. Like earlier, The attribute is only valid with pointer and reference types.

[vikramRH]

gentle ping....

[AaronBallman]

Sorry for missing this, it was in Draft status and so I wasn't looking at it for reviews. :-)

This is sort of heading in the direction I think it should go. I did have a suggestion though.

[AaronBallman]

Instead of taking an int for the second argument, I think it would make more sense to do:

class AddressSpace<int AddrSpaceValue> {
  int SpaceNum = AddrSpaceValue;
}

// Use whatever the correct values are, I just used random numbers
def Global : AddressSpace<12>;
def Constant : AddressSpace<1002>;

class ArgWithAddressSpace<int ArgIdx, AddressSpace Num> : IndexedAttribute<"", ArgIdx< {
  AddressSpace AddrSpace = Num;
}
...
let Attributes = [ArgWithAddressSpace<0, Global>, NoThrow];

WDYT?

[vikramRH]

handled #122873