[PATCH] D30364: AArch64 : Add PreferCSEL feature.

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D30364#690221, @flyingforyou wrote:

> Renato, before we add new feature for core, we use the code likes
>
> IsExynosM1 or IsCortexA57 ...
>
> But after discussion, we move to use Feature things instead of above code.


This is correct.

> I am not sure that cost model can cover all the things what uArchitecture's all the specific details.

I have to agree with you and James. We have had discussions about this in the past and in some cases, it's possible, in others, it's not.

This is because the current ISel has some thresholds which can be used as very rudimentary cost models, but unless we have a way to do this generically, any work around will be as hacky as a feature.

However, this patch goes one extra step: It changes the table-gen definitions to *forbid* CSEL based on a feature called "PreferCSEL". This is confusing and a misrepresentation of what the flag actually means.

IIRC, other flags have C++ code in the style:

  if (FeaturePreferFoo)
    emitFOO();
  else
    emitBAR();

which, for ISel purposes, it's the same effect as your table-gen solution. However, this is not true for the assembler. CSEL is valid on that core, and if I try to assemble files with it in this new table-gen, it will bail saying that the instruction requires "DontUseCSEL", which in itself is quite opaque and meaningless to the user.

So, using the flag as a dynamic choice during ISel only should be fine for the time being, but not adding it to the table-gen as a hard requirement.

Makes sense?

--renato


https://reviews.llvm.org/D30364