[PATCH] D100721: [SCEVExpander] Try to create ASHR instr for expanded SCEV expr.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a subscriber: craig.topper.
lebedev.ri added a comment.

In D100721#2697176 <https://reviews.llvm.org/D100721#2697176>, @xbolva00 wrote:

>>> revert
>
> +1

If you don't mind me asking, i forgot when was the last time
i have seen any (coherent?) message longer than two phrases from you;
all recent comments are simply non-comments flippantly reiterating
the last point made in whatever thread.
What's up with that?

In D100721#2697197 <https://reviews.llvm.org/D100721#2697197>, @fhahn wrote:

> In D100721#2697174 <https://reviews.llvm.org/D100721#2697174>, @nikic wrote:
>
>> I would recommend reverting rGec54867df5e7f20e12146e628af34f0384308bcb <https://reviews.llvm.org/rGec54867df5e7f20e12146e628af34f0384308bcb>.
>
> That's certainly a good way forward for now IMO, as I definitely agree that the improved modeling would be a lot of work and even then it is not clear how feasible this would be for real-world cases.
>
> @lebedev.ri Given that ec54867df5e7 <https://reviews.llvm.org/rGec54867df5e7f20e12146e628af34f0384308bcb>  can have a noticeable negative impact on performance, WDYT about a revert?

Hmm, so the problem is that we *can't* reconstruct `ashr`/`ashr exact` from it's SCEV model:

  $ ./alive-tv /tmp/test.ll --bidirectional
  
  ----------------------------------------
  define i8 @src(i8 %x) {
  %0:
    %r = ashr exact i8 %x, 4
    ret i8 %r
  }
  =>
  define i8 @tgt(i8 %x) {
  %0:
    %abs_x = abs i8 %x, 0
    %div = udiv exact i8 %abs_x, 16
    %t0 = smax i8 %x, 255
    %t1 = smin i8 %t0, 1
    %r = mul nsw i8 %div, %t1
    ret i8 %r
  }
  Transformation seems to be correct!
  
  These functions seem to be equivalent!

  e$ ./alive-tv /tmp/test.ll --bidirectional
  
  ----------------------------------------
  define i8 @src(i8 %x) {
  %0:
    %abs_x = abs i8 %x, 0
    %div = udiv i8 %abs_x, 16
    %t0 = smax i8 %x, 255
    %t1 = smin i8 %t0, 1
    %r = mul nsw i8 %div, %t1
    ret i8 %r
  }
  =>
  define i8 @tgt(i8 %x) {
  %0:
    %r = ashr i8 %x, 4
    ret i8 %r
  }
  Transformation doesn't verify!
  
  ERROR: Value mismatch
  
  Example:
  i8 %x = #xc8 (200, -56)
  
  Source:
  i8 %abs_x = #x38 (56)
  i8 %div = #x03 (3)
  i8 %t0 = #xff (255, -1)
  i8 %t1 = #xff (255, -1)
  i8 %r = #xfd (253, -3)
  
  Target:
  i8 %r = #xfc (252, -4)
  Source value: #xfd (253, -3)
  Target value: #xfc (252, -4)

I don't recall my thoughts on that back then, but it's clearly bad.
That being said we really should model `ashr exact` in SCEV,
that comes up all the time you want to compute the distance between to pointers e.g.

So i'll revert for now, thanks!



================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:828-829
+  // to match the pattern and emit a ASHR instruction directly.
+  if (IsSignumExpr(S->getOperand(1), X1) &&
+      IsDivAbs(S->getOperand(0), X2, Amount) && X1 == X2)
+    return Builder.CreateAShr(expandCodeForImpl(X1, Ty, false),
----------------
i think this matcher should be ,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100721