[PATCH] D85524: [Loads] Add canReplacePointersIfEqual helper.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/Loads.cpp:524
+    return C->isNullValue() ||
+           isDereferenceablePointer(B, Ty->getPointerElementType(), DL, CtxI);
+  }
----------------
fhahn wrote:
> nikic wrote:
> > nikic wrote:
> > > efriedma wrote:
> > > > This check is full of holes... but I guess it's a good thing if we can start marking the places in transforms where this matters without worrying too much about the initial performance impact.
> > > This check looks fundamentally incompatible with opaque pointers.
> > Maybe just check derefability of a single byte, instead of the pointer element type?
> > This check is full of holes... but I guess it's a good thing if we can start marking the places in transforms where this matters without worrying too much about the initial performance impact.
> 
> Yes, I added a note. The holes are intentional, to avoid causing too many issues at once and allowing us to gradually tighten the restrictions. FWIW if you require both pointers to be dereferenceable, there are binary changes in ~50 programs out of MultiSource/SPEC2000/SPEC2006, so not too terrible either. But it's probably better to take things in steps.
> 
> > This check looks fundamentally incompatible with opaque pointers.
> > Maybe just check derefability of a single byte, instead of the pointer element type?
> That might work, but we probably should do this for all uses of `isDereferenceablePointer`, potentially as follow-up?
> 
> That might work, but we probably should do this for all uses of isDereferenceablePointer, potentially as follow-up?

To be clear, the problem is not in the isDereferenceablePointer() API, but in the way it is used in this specific case. Other uses of this API correctly pass in a load type, not a pointer element type. As you do not have access to a load type here, checking for a single byte seems like the best you can do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85524