[PATCH] D75072: [SCEV] Record NI types in add exprs

[Keno Fischer via Phabricator via llvm-commits]

loladiro created this revision.
loladiro added reviewers: sanjoy, reames, vtjnash, vchuravy.
Herald added subscribers: llvm-commits, javed.absar, hiraditya.
Herald added a project: LLVM.

This fixes a case where loop-reduce introduces ptrtoint/inttoptr for
non-integral address space pointers. Over the past several years, we
have gradually improved the SCEVExpander to actually do something
sensible for non-integral pointer types. However, that obviously
relies on the expander knowing what the type of the SCEV expression is.
That is usually the case, but there is one important case where it's
not: The type of an add expression is just the type of the last operand,
so if the non-integral pointer is not the last operand, later uses of
that SCEV may not realize that the given add expression contains
non-integral pointers and may try to expand it as integers.

One interesting observation is that we do get away with this scheme in
shockingly many cases. The reason for this is that SCEV expressions
often have an `scUnknown` pointer base, which our sort order on the
operands of add expressions sort behind basically everything else,
so it usually ends up as the last operand.

One situation where this fails is included as a test case. This test
case was bugpoint-reduced from the issue reported at
https://github.com/JuliaLang/julia/issues/31156. What happens here
is that the pointer base is an scAddRec from an outer loop, plus an
scUnknown integer offset. By our sort order, the scUnknown gets sorted
after the scAddRec pointer base, thus making an add expression of these
two operands have integer type. This then confuses the expander, into
attempting to expand the whole thing as integers, which will obviously
fail when reaching the non-integral pointer.

I considered a few options to solve this, but here's what I ended up
settling on: The AddExpr class gains a new subclass that explicitly
stores the type of the expression. This subclass is used whenever one
of the operands is a non-integral pointer. To reduce the impact for the
regular case (where the SCEV expression contains no non-integral
pointers), a bit flag is kept in each flag expression to indicate
whether it is of non-integral pointer type (this should give the same
answer as asking if getType() is non-integral, but performing that
query may involve a pointer chase and requires the DataLayout). For
add expressions that flag is also used to indicate whether we're using
the subclass or not. This is slightly inefficient, because it uses
the subclass even in the (not uncommon) case where the last operand
does actually accurately reflect the non-integral pointer type. However,
it didn't seem worth the extra flag bit and complexity to do this
micro-optimization.

I had hoped that we could additionally restrict mul exprs from
containing any non-integral pointers, and also require add exprs to
only have one operand containg such pointers (but not more), but this
turned out not to work. The reason for this is that SCEV wants to
form differences between pointers, which it represents as `A + B*-1`,
so we need to allow both multiplication by `-1` and addition with
multiple non-integral pointer arguments. I'm not super happy with
that situation, but I think it exposes a more general problem with
non-integral pointers in LLVM. We don't actually have a way to express
the difference between two non-integral pointers at the IR level.
In theory this is a problem for SCEV, because it means that we can't
materialize such SCEV expression. However, in practice, these
expressions generally have the same base pointer, so SCEV will
appropriately simplify them to just the integer components.
Nevertheless it is a bit unsatisfying. Perhaps we could have an
intrinsic that takes the byte difference between two pointers to the
same allocated object (in the same sense as is used in getelementptr),
which should be a sensible operation even for non-integral pointers.
However, given the practical considerations above, that's a project
for another time. For now, simply allowing the existing pointer-diff
pattern for non-integral pointers seems to work ok.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75072

Files:
  llvm/include/llvm/Analysis/ScalarEvolution.h
  llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h
  llvm/lib/Analysis/ScalarEvolution.cpp
  llvm/test/Transforms/LoopStrengthReduce/nonintegral.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75072.246273.patch
Type: text/x-patch
Size: 14383 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200224/1f255b4e/attachment.bin>