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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D50222#1244716, @hermord wrote:

> @lebedev.ri Sorry, couldn't find any instances of `4 x i16` in the nonsplat tests; I think I only had those in the splat ones.


Hm, could be. I don't see them either now.



================
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)
----------------
hermord wrote:
> 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.
This current `BuildUREMEqFold()` does not `adds nodes to the worklist directly` as far as i can see?

Well, i had the third variant in mind.
1. Rename this function to something like `BuildUREMEqFold_REAL()` (bad name, just an example)
2. Add a new wrapper `BuildUREMEqFold()` here in this file, right after `BuildUREMEqFold_REAL()`.
So nothing changes for the callers.
And the wrapper will do all that stuff about receiving the vector of newly created `SDNodes`,
and adding them to the worklist.


Repository:
  rL LLVM

https://reviews.llvm.org/D50222