[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D60583#1529937 <https://reviews.llvm.org/D60583#1529937>, @fpetrogalli wrote:

> In D60583#1529878 <https://reviews.llvm.org/D60583#1529878>, @jdoerfert wrote:
>
> > Why/Where did we decide to clobber the attribute list with "non-existent function names"?
>
>
> The existence of those symbols is guaranteed by the "contract" stipulated via the Vector Function ABI. They cannot be added explicitly by the front-end as `define`s because they would be removed before reaching the vectorizer.


That is not a good argument. Afaik, there are multiple ways designed to keep symbols alive, e.g., `@llvm.used`.

>> I don't think an attribute list like this:
>>  `attributes #1 = { "_ZGVsM2v_foo" "_ZGVsM32v_foo" "_ZGVsM4v_foo" "_ZGVsM6v_foo" "_ZGVsM8v_foo" "_ZGVsMxv_foo" ... `
>>  is helpful in any way, e.g., this would require us to search through all attributes and interpret them one by one.
> 
> Agree. This is what was agreed : http://lists.llvm.org/pipermail/cfe-dev/2016-March/047732.html
> 
> The new RFC will get rid of this list of string attributes. It will become something like:
> 
>   attribute #0 = { declare-variant="comma,separated,list,of,vector,function,ABI,mangled,names" }.
> 
> 
> 
> 
>> This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.
> 
> I can assure you that's not the case.
> 
> The code in this patch is what it is because it is based on previous (accepted) RFC originally proposed by other people and used by VecClone: https://reviews.llvm.org/D22792
> 
> As you can see in the unit tests of the VecClone pass, the variant attribute is added as follows:
> 
>   attributes #0 = { nounwind uwtable "vector-variants"="_ZGVbM4_foo1,_ZGVbN4_foo1,_ZGVcM8_foo1,_ZGVcN8_foo1,_ZGVdM8_foo1,_ZGVdN8_foo1,_ZGVeM16_foo1,_ZGVeN16_foo1"  .... }

I get that it was discussed three years ago and I get that it was accepted then.
My confusing stems from the fact that it was committed just now, three years later, but shortly before the new RFC basically proposed a different encoding.

> Nothing in LLVM is using those attributes at the moment, that might be the reason why the string attribute have not yet been moved to a single attribute.

That means we can easily change the encoding, and I strongly believe we should.

Given that you want a different encoding, I don't know if I have to list reasons why I dislike this one (direct names as attributes). Though, I can do so if we want to discuss it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583