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

[Dmytro Shynkevych via Phabricator via llvm-commits]

hermord added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3606
+                                       SelectionDAG &DAG,
+                                       std::vector<SDNode *> &Created) const {
+  // fold (seteq/ne (urem N, D), 0) -> (setule/gte (rotr (mul N, P), K), Q)
----------------
lebedev.ri wrote:
> hermord wrote:
> > RKSimon wrote:
> > > lebedev.ri wrote:
> > > > This should be `SmallVectorImpl<SDNode *> &Created` like in all the other cases.
> > > Isn't this still missing?
> > The approach is different now: `BuildUREMEqFold` is now called from `TargetLowering::SimplifySetCC` and not from `DAGCombiners::visitREM`, and l tried to match the style of the other helpers called from there (like `simplifySetCCWithAnd`).
> Looking at the code on which this was originally based, i think i was slightly wrong.
> This should still have a pointer to smallvector, and it should push back the newly created `SDValue`s.
> And this function should be called from a new helper function that would make proper use of that.
> 
> See `TargetLowering::BuildSDIVPow2()` (which is the implementation), vs `DAGCombiner::BuildSDIVPow2()` (which is the wrapper).
Ah sorry, just to clarify, there used to be a `DAGCombiner::BuildREMEqFold` in an earlier version of this patch, but my rationale for removing it was that:
1. We wanted to generate this fold when visiting the SETCC and not the UREM.
2. `DAGCombiner::visitSetCC` delegates to `DAGCombiner::SimplifySetCC`, which is just a stub calling `TargetLowering::SimplifySetCC`.
3. `TargetLowering::SimplifySetCC` adds nodes to the worklist directly and doesn't return an `SDValue` vector.

If it's better to mimic `BuildSDIVPow2` and have a wrapper, I could:

1. Have `TargetLowering::SimplifySetCC` call the wrapper from `DAGCombiner`, at the risk of this being slightly confusing (I don't believe anything else in that function does that?)
2. Remove the call to `TargetLowering::BuildUREMEqFold` from `TargetLowering::SimplifySetCC` and add a call to the wrapper instead to `DAGCombiner::SimplifySetCC` or `DAGCombiner::visitSETCC`. Currently neither actually contains any case-specific simplification logic, so I'm not sure if I should change that.


Repository:
  rL LLVM

https://reviews.llvm.org/D50222