[PATCH] D30364: AArch64 : Add FastCSEL feature.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:46
 
+def HasSlowCSEL : Predicate<"!Subtarget->hasFastCSEL()">;
+
----------------
I think that the proposed name change would make this closer to the intended meaning:


```
def HasFastCSINCV : Predicate<"!Subtarget->HasSlowCSINCV()">;
```


================
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),
----------------
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]>;
```


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:79-80
 
+  // If true, CSEL will be favored over CSINV or CSINC.
+  bool HasFastCSEL = false;
+
----------------
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;




https://reviews.llvm.org/D30364