[PATCH] Target-specific attributes

[Alp Toker]

On 04/01/2014 22:40, Aaron Ballman wrote:
> On Sat, Jan 4, 2014 at 4:55 PM, Alp Toker <alp at nuanti.com> wrote:
>> Hi Aaron,
>>
>> The identical-looking blocks look like a straight code-move so I haven't
>> examined them closely (if there are any significant changes in there, do
>> mention). Here's the rest..
> You have it correct -- the code simply moved around, but has no
> substantive changes.
>
>>> +// Defines targets for target-specific attributes. The list of strings
>>> should
>>> +// specify architectures for which the target applies, based off the
>>> ArchType
>>> +// enumeration in Triple.h.
>>> +class TargetArch<list<string> arches> {
>>> +  list<string> Arches = arches;
>>> +  list<string> OSes;
>>> +}
>>> +def TargetARM : TargetArch<["arm", "thumb"]>;
>>> +def TargetMSP430 : TargetArch<["msp430"]>;
>>> +def TargetX86 : TargetArch<["x86"]>;
>>> +def TargetX86Win : TargetArch<["x86", "x86_64"]> {
>>> +  let OSes = ["Win32", "MinGW32"];
>>> +}
>>> +def TargetMips : TargetArch<["mips", "mipsel"]>;
>>> +
>>
>> Feels like it'd be more TableGen-ish to apply TargetArch<> directly to the
>> instances without listing these utility wrappers. Maybe only provide the
>> defs for TargetMips and TargetARM that add value? It's your call though if
>> you prefer to keep it this way -- I don't have a strong opinion here.
> I don't have a strong opinion either way, but I have a slight
> preference for being more declarative like the patch is doing. The
> rationale being: as people add more target-specific attributes, these
> will be reused, which makes it easier to modify the targets (such as
> adding thumb to arm, etc).

Quite reasonable, on balance I'm fine with this.

>
>>> -def DLLExport : InheritableAttr, TargetSpecificAttr {
>>> +def DLLExport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
>>>     let Spellings = [Declspec<"dllexport">];
>>>     let Subjects = SubjectList<[Function, Var]>;
>>>   }
>>>
>>> -def DLLImport : InheritableAttr, TargetSpecificAttr {
>>> +def DLLImport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
>>>     let Spellings = [Declspec<"dllimport">];
>>>     // Technically, the subjects for DllImport are Function and Var, but
>>> there is
>>>     // custom semantic handling required when MicrosoftExt is true.
>>
>> Any reason for dllimport / dllexport to be x86 target attributes?
> Because they were x86 previously. This patch was going for feature
> parity with the existing functionality.
>
>> That quirk
>> looks like a historical leftover that can be handled by the ["Win32",
>> "MinGW32"] predicate alone thanks to your changes. MSDN documents them as
>> being general language features not specific to x86 and I believe they apply
>> to other Microsoft architectures equally.
> That I could definitely believe, but should probably be a separate
> patch. Good catch, though!

Fair enough. I'm looking forward to that cleanup as well.

LGTM

Alp.


>
> ~Aaron

-- 
http://www.nuanti.com
the browser experts