[PATCH] D86578: [TargetLowering] Combine known bits for icmp in SimplifySetCC (PR41182)

[Matt D'Arcangelo via Phabricator via llvm-commits]

darcangelo.matthew added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14059
+      // unconditionally take the branch, remove all other successors of CurBB
+      auto SuccMBB = CurMBB->succ_begin();
+      while (SuccMBB != CurMBB->succ_end()) {
----------------
craig.topper wrote:
> darcangelo.matthew wrote:
> > craig.topper wrote:
> > > I'm not sure if we can really do this from SelectionDAG. Is there anywhere else that does this in tree?
> > > I'm not sure if we can really do this from SelectionDAG. Is there anywhere else that does this in tree?
> > 
> > I found the UnreachableMachineBlockElim pass which may be able to be enhanced for this, but I'm still looking
> Sorry, I meant is there anywhere else that does something like this while SelectionDAG exists?
Thanks for elaborating, sorry for the confusion. I don't believe there is another place that does something like this while SelectionDAG exists. It seems like BranchFolding might be a good place for me to handle this?

For some additional context, without this change if we have 
```define i32 @simple(i32 %x) nounwind {
  %1 = and i32 %x, 2147483648
  %2 = icmp sle i32 %1, 0
  br i1 %2, label %yes, label %no

yes:                    ; will always get here
  ret i32 1
no:
  ret i32 2
}```
 we get
```
## %bb.0:
         xorl %eax, %eax
         testb %al, %al
	 jne	LBB0_2
## %bb.1:                               ## %yes
	movl	$1, %eax
	retq
LBB0_2:                                 ## %no
	movl	$2, %eax
	retq```
so jne will never be taken instead of just removing the branch completely


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3998
           C1 == APInt::getSignedMinValue(OperandBitSize)) {
-        SDValue ConstMinusOne =
-            DAG.getConstant(APInt::getAllOnesValue(OperandBitSize), dl,
-                            N1.getValueType());
+        SDValue ConstMinusOne = DAG.getConstant(
+            APInt::getAllOnesValue(OperandBitSize), dl, N1.getValueType());
----------------
craig.topper wrote:
> Is this just a formatting change?
Yes this is just formatting, I must have messed up the merge when I rebased and missed this. Sorry about that, should be fixed now


================
Comment at: llvm/test/CodeGen/X86/test-shrink.ll:574
   ret void
 no:
   ret void
----------------
craig.topper wrote:
> darcangelo.matthew wrote:
> > craig.topper wrote:
> > > Can we pre-commit the IR changes here?
> > Thank you for taking a look at the changes. 
> > 
> > > Can we pre-commit the IR changes here?
> > 
> > Sorry, would you be able to elaborate what you mean here? Do you mean separate these out into another pre-commit review?
> I went ahead and modified the tests in trunk. Can you rebase this patch?
> I went ahead and modified the tests in trunk. Can you rebase this patch?

Sure can, thanks for updating them. Sorry for the confusion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86578