[PATCH] D30364: AArch64 : Add PreferCSEL feature for Exynos-M3.

[Evandro Menezes via llvm-commits]

Hi, James.

I guess that I confused Pat and Alias.  My bad.


-- 
Evandro Menezes

On 03/03/2017 02:42 AM, James Molloy wrote:
> Hi Evandro,
>
> Pat<>s are never used in the assembler, unless something fundamental has changed recently! What the assembler sees is controlled by Predicate<>s on the Instruction defs. The Pat<> classes only apply to the instruction selector.
>
> So it is completely feasible to disable instruction selection patterns but keep the instruction selectable and disassemblable. Junmo’s first patch does this:
>
> def : Pat<(AArch64csel (i32 0), (i32 1), (i32 imm:$cc), NZCV),
>            (CSINCWr WZR, WZR, (i32 imm:$cc))>, Requires<[DontUseCSEL]>;
>
> With the Requires<> being added by Junmo. All this does is disable selection of CSINCWr from a pattern involving a CSEL. CSEL is still a valid instruction, and so is CSINC, so the assembler will parse and emit both without warning or error.
>
> What I would say however is that when I discussed this with Junmo earlier, he suggested it was the CSINC/CSINV instructions that were slow on Exynos-M3, *not* CSEL. That is backed up by his code change, but *not* by the naming of the feature, “DontUseCSEL”. In fact, CSINC has been disabled with the feature “DontUseCSEL”, which is confusing at best. CSEL’s patterns aren’t even disabled by this option, so it’s not well named.
>
> James
>
>> On 2 Mar 2017, at 21:11, Evandro Menezes <e.menezes at samsung.com> wrote:
>>
>> Hi, James.
>>
>> I think that the question here is if a Pat should be gated by a SubtargetFeature.  As Renato pointed out to me, Pat is also used by the assembler, so, when using one to optimize code, it also ends up optimizing assembly source code, which is not desirable.
>>
>> Not that a Pat should never be used to result in better code, but if the the logic needs to be even slightly complex, perhaps it should be moved to TargetLowering, where not only is complex logic more legible in C++ than in TableGen, but it's also possible to access other resources more easily, such as the cost model, in order to make the best decision when lowering to the target.
>>
>> Something along these lines could help us abusing SubtargetFeature after we decided to refrain from abusing isTarget().
>>
>> Cheers,
>>
>> --
>> Evandro Menezes
>>
>> On 03/01/2017 02:24 AM, James Molloy via Phabricator wrote:
>>> jmolloy added a comment.
>>>
>>> Hi Renato,
>>>
>>>>    If it does, but the cost is bad (for whatever reason), this really really should be in the cost model...
>>> For my own understanding - how does the cost model impact the choices ISel makes? I didn't think it could. Or are you saying *in addition to* disabling the relevant Pat<>s with a feature, this should *also* be reflected in the cost model?
>>>
>>> James
>>>
>>>
>>> https://reviews.llvm.org/D30364
>>>
>>>
>>>
>>>
>>>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.