[PATCH] D137422: [DAGCombine] Generalize foldSelectCCToShiftAnd

[Mikhail Gudim via Phabricator via llvm-commits]

mgudim marked 3 inline comments as done.
mgudim added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:24377
+  //     select_cc setgt X, 0, X, 0 -> and (not (sra X, size(X)-1)), X
+  //     select_cc setlt X, 1, 0, X -> and (not (sra X, size(X)-1)), X
   EVT XType = N0.getValueType();
----------------
reames wrote:
> This part relies on an additional observation that X == 0, the two sides of the select return the same value.   
> 
> There's an interesting question about whether we want to canonicalize these towards the other form.  0 and -1 are likely cheaper constants on at least some architectures.  Might be worth an experiment to see how that plays out.
> 
> Assuming we don't, this can be a separate change with a clarified comment to emphasize the additional fact stated at the top of this comment.  
Ok, let's leave at as a separate change for later.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:24468
     return SDValue();
 
+  if (!isNullConstant(N3) && isNullConstant(N2)) {
----------------
I thought about doing this reversal all the way up in `SimplifySelectCC`.  This may enable other transformations that previously assumed the canonicalized form. For example:

```
  // Fold select_cc setgt X, -1, C, ~C -> xor (ashr X, BW-1), C
  // Fold select_cc setlt X, 0, C, ~C -> xor (ashr X, BW-1), ~C

```

I haven't  tried it yet, but do you think this is worth exploring?


================
Comment at: llvm/test/CodeGen/AArch64/arm64-fmax.ll:61
 ; CHECK-LABEL: test_integer:
 ; CHECK:       // %bb.0:
+; CHECK-NEXT:    bic x0, x0, x0, asr #63 
----------------
I need to update 59 tests for these change. In some tests the checks were not auto-generated. What is the usual procedure to update many tests?  Is it ok to insert auto-generated checks in the tests that didn't have them before?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137422/new/

https://reviews.llvm.org/D137422