[PATCH] D85332: [SCCP] Do not replace deref'able ptr with un-deref'able one.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D85332#2197623 <https://reviews.llvm.org/D85332#2197623>, @efriedma wrote:

> LLVM's memory model isn't completely settled with respect to how to determine the "base" of a pointer; see https://bugs.llvm.org/show_bug.cgi?id=34548 etc. But consensus is that the general transform you're describing isn't legal without proving more about the pointer. (e.g. that the two pointers have the same base, or the replaced value isn't used for any memory operations).

Great, thanks for the reference.

This patch is aimed as a first step towards incrementally improving the current situation by avoiding replacements for which there is agreement that they are problematic. The narrow initial case would be replacing a dereferenceable pointer with a clearly un-dereferenceable one.

If there is consensus on the approach, I think it would make sense to adjust the patch roughly as follows:

1. Add a `canReplaceDueToPointerEqualilty(Ptr1, Ptr2)` which returns true if we can replace Ptr1 with Ptr2 if they are considered equal or false if not.
2. The initial implementation will be incomplete and only reject certain cases (rather than only allowing cases we know are fine (e.g. both pointers dereferenceable and from the same underlying object)
3. Update GVN/SCCP/CPV to use `canReplaceDueToPointerEqualilty`.

Does that make sense?



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1357
+          BasicAAResult::DecomposeGEPExpression(C, Dec, DL, nullptr, nullptr);
+          auto *BaseTy = Dec.Base->getType()->getPointerElementType();
+          if (BaseTy->isArrayTy() &&
----------------
efriedma wrote:
> I'm not sure I completely follow what this code is doing, but using getPointerElementType() like this is probably wrong.
My intention here is to catch the case where we create a pointer to the 'one-past-the-last-element', which clearly is not dereferenceable. 

As implemented above, it probably only works for GEPs with pointers to array types as bases. So this would need further refinement to make sure we actually use the right base when dealing with nested data types. Is that what you were referring to or is there something else I am missing? Do you know if there are any helpful utilities for doing such a thing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85332