[PATCH] D149383: [SelectionDAG][WIP] Add support for evaluating SetCC based on knownbits

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4288-4289
+
+      // We aren't able to constant fold with known bits but can either 1) make
+      // conditions stronger (i.e ule -> ult) or 2) simplify with
+      // isKnownNeverZero if RHS is zero.
----------------
foad wrote:
> Just curious: does "strengthening" conditions like this actually generate better code? Is it something we do elsewhere in the compiler?
No worse in fact. This was really motivated by wanting to improve the knownbits analysis in selectiondag but having no good way to test it.

I think at the moment, since we don't have the infrastructure to fold basic blocks when `icmp; br` conditions are known `true/false` we end up with regressions.

Not sure exactly where this is going to go unless we add that infrastructure.


================
Comment at: llvm/test/CodeGen/X86/avx512-mask-op.ll:4670
+; KNL-NEXT:    movb $1, %al
+; KNL-NEXT:    testb %al, %al
 ; KNL-NEXT:    je LBB77_1
----------------
RKSimon wrote:
> Whats going on here?
Its what you thought was a bad merge in the knownbits impl.

```
    if (computeKnownBits(Op.getOperand(0)).One[0], Depth + 1)
      return true;
```
See the bug?

Should be:
```
    if (computeKnownBits(Op.getOperand(0), Depth + 1).One[0])
      return true;
```

Suprised the former is accepted as an expression (not a clang warning/error). Its 

`if (expr_A, expr_B)`


================
Comment at: llvm/test/CodeGen/X86/fold-rmw-ops.ll:1372
   store i64 %or, ptr @g64
   %cond = icmp eq i64 %or, 0
   br i1 %cond, label %a, label %b
----------------
RKSimon wrote:
> goldstein.w.n wrote:
> > RKSimon wrote:
> > > comparing against zero in all these or-with-imm tests just seems to be a copy+paste from the other logic ops in this file - maybe change it to something that isn't constant foldable (test for -ve?)
> > re: 'test for -ve?' hmm?
> > 
> > But would generally prefer to add new tests than modify existing.
> 'test for -ve' === 'test for negative'
> 
> Adding additional tests would be fine.
As in add a new test file? This isn't a new file for the series, its just affected by the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149383