[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:
> goldstein.w.n wrote:
> > 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.
> You're adding code to the compiler to change these setcc conditions, but not actually make anything better? I do not think that is a good idea.
So worse might be a bit of a exageration. It's worse for some IR that the middle-end would never pass to us.

I was thinking maybe we could do this for SETCC not used by `br`. That case we see improvement. What I need to do I think though, do a little work seeing if any of the cases this changes are cases generated by the backend (i.e cases that wouldn't normally be cleaned up by the middle-end).

But I agree, in the current state its not submitable.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4302
+        if (!Res && KnownRHS.isZero() && DAG.isKnownNeverZero(N0))
+          Res = false;
+        break;
----------------
foad wrote:
> RKSimon wrote:
> > I'm not sure this is always false?
> It's confusing but I think it's correct. From this point onwards, `Res` represents whether LHS and RHS are //equal//, irrespective of `Cond`.
I'll make it a new variable.


================
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:
> > > 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.
> I'd prefer that these tests were adjusted, the icmp_eq vs 0 was just a dumb copy + paste - but if you don't want to do that, duplicating these OR tests immediately below with a icmp_sgt 0 would be OK
Ah, okay. Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149383