[PATCH] D50222: [CodeGen] [SelectionDAG] More efficient code for X % C == 0

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some more high-level nits/thoughs:

- Other than `-Os`, do we always want to do this, for every target, with no customization hook?
- Right now the tests look good (other than the run-line question), please commit them now.
- Similarly, if we want to do it for other arches, also commit a copy of that same `rem-seteq.ll` test for aarch64 at least.
- I think this should be further split into two reviews, unsigned case first :/



================
Comment at: include/llvm/CodeGen/TargetLowering.h:3698
                                                const SDLoc &DL) const;
+  SDValue BuildREMEqFold(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
+                         DAGCombinerInfo &DCI, const SDLoc &DL) const;
----------------
Separate with newline?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:2780-2782
+    SDValue Folded = BuildREMEqFold(VT, N0, N1, Cond, DCI, dl);
+    if (Folded)
+      return Folded;
----------------
```
if (SDValue Folded = BuildREMEqFold(VT, N0, N1, Cond, DCI, dl))
  return Folded;
```


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3723-3725
+  // fold (seteq/ne (urem N, D), 0) -> (setule/gte (rotr (mul N, P), K), Q)
+  // fold (seteq/ne (srem N, D), 0)
+  //          -> (setule/gte (rotr (add (mul N, P), Q''), K), (srl Q', K-1))
----------------
The produced patterns are rather different between unsigned and signed cases.
I think this should be at least done in two steps (two reviews, unsigned first),
and likely these should be done by two separate functions.



================
Comment at: test/CodeGen/X86/rem-seteq.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i386-unknown-unknown | FileCheck %s
+
----------------
Any reason this is targeting `i386` specifically?
I //think// we should test:
```
; RUN: llc -mtriple=i686-unknown-linux-gnu                < %s | FileCheck %s --check-prefixes=CHECK,X86
; RUN: llc -mtriple=x86_64-unknown-linux-gnu              < %s | FileCheck %s --check-prefixes=CHECK,X64,NOBMI2
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -mattr=+bmi2 < %s | FileCheck %s --check-prefixes=CHECK,X64,BMI2
```


Repository:
  rL LLVM

https://reviews.llvm.org/D50222