[PATCH] D30364: AArch64 : Add FastCSEL feature.

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

A few other things missing:

- A core where this is set by default? Otherwise, why do we have this?
- Tests. Regardless of cores, tests with and without that feature must exist.

cheers,
--renato



================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:1137-1138
 
 def : Pat<(AArch64csel (i32 0), (i32 1), (i32 imm:$cc), NZCV),
-          (CSINCWr WZR, WZR, (i32 imm:$cc))>;
+          (CSINCWr WZR, WZR, (i32 imm:$cc))>, Requires<[HasSlowCSEL]>;
 def : Pat<(AArch64csel (i64 0), (i64 1), (i32 imm:$cc), NZCV),
----------------
kristof.beyls wrote:
> With the name change I'm proposing, I think this reads more natural and is easier to understand:
> 
> ```
> def : Pat<(AArch64csel (i32 0), (i32 1), (i32 imm:$cc), NZCV),
>           (CSINCWr WZR, WZR, (i32 imm:$cc))>, Requires<[HasFastCSINCV]>;
> ```
Yup.


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:79-80
 
+  // If true, CSEL will be favored over CSINV or CSINC.
+  bool HasFastCSEL = false;
+
----------------
kristof.beyls wrote:
> Sorry to be late in my review feedback here.
> 
> Wouldn't "HasSlowCSINCV" be more natural and make the intent a tiny bit easier to understand?
> 
> e.g.
>   // If true, CSINV or CSINC will not be generated, using CSEL instead.
>   bool HasSlowCSINCV = false;
> 
> 
I agree with Kristof here.


https://reviews.llvm.org/D30364