[PATCH] D137422: [DAGCombine] Generalize foldSelectCCToShiftAnd

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:24369
+  //     select_cc setgt X, -1, A, 0 -> and (not (sra X, size(X)-1)), A
+  //     select_cc setlt X, 0, 0, A -> and (not (sra X, size(X)-1)), A
+  //
----------------
A style, and process suggestion.

These first patterns can be handled via the common idiom of reading the operands, and then swapping the operand values before the main transform if required.

This would stand along well as it's own change.   This is the part which currently matches your change description anyways.


================
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();
----------------
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.  


================
Comment at: llvm/test/CodeGen/RISCV/select-to-shift-and.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+
----------------
mgudim wrote:
> craig.topper wrote:
> > mgudim wrote:
> > > This is a target-independent optimization, so I am not sure how to properly test it. I included this file only to demonstrate how the optimization works.
> > > 
> > > Please let me know what you think a proper place / form for this test should be.
> > Why did you stop on MIR instead of assembly?
> Since we are testing something in `ISel` I thought it would make sense to run only `ISel`. 
> 
> I do see that most of other tests go all the way to assembly. I am fine with either way. 
For codegen tests, it is expected to show assembly.  That's normal practice, and you should follow that.  Please replace update_mir with update_llc_test_checks.py


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

https://reviews.llvm.org/D137422