[PATCH] D111186: [SCEV] Infer flags from add/gep in any block

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D111186#3045982 <https://reviews.llvm.org/D111186#3045982>, @reames wrote:

> In D111186#3045847 <https://reviews.llvm.org/D111186#3045847>, @nikic wrote:
>
>>> The comment is also suspect as well in that we're in the middle of constructing a SCEV for I. As such, we're going to visit all operands *anyways*.
>>
>> I can only make a guess here, but what this might be referring to is the fact that SCEV construction from IR will coalesce add/sub and mul expressions into a single call of getAddExpr/getMulExpr rather than building up a chain of binary adds/muls. Effectively, the change you do here defeats that (for the case where the IR instruction has flags, even if they are inapplicable), because you will end up calling getSCEV on each individual add due to the operand fetch in the poison check.
>
> So, while reasonable, this isn't quite right or at least isn't so any longer.  When constructing an add reduction tree, we will aggressively collapse into a single add node, dropping flags as needed.  This is done inside getAddExpr.  The code that runs during parsing - which goes out of it's way to separate by flag type - appears to just be canonicalized back into the flattened form )if not all flags match).  Now, maybe there's a difference in the number of nodes from the interior of the tree formed, but that cost should be *very* minimal.  (As for one thing, how would you construct an add tree with a non-SCEVable operand type?)

I'm not sure I understand what you're saying here. Maybe easier to talk about an example:

  %x = add %a, %b
  %y = add nuw %x, %c

Let's say we call `getSCEV(%y)` in a position where flags could not be transferred. Prior to this change, this would call `getAddExpr(S(%a), S(%b), S(%c))`. After this change it will additionally call `getAddExpr(S(%a), S(%b))` as part of the poison check, when evaluating the `%x` operand. This additional expression will go unused (beyond the poison check). Previously, this would only happen if the flags could actually be transferred.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111186