[PATCH] D64258: [InferFuncAttributes] extend 'dereferenceable' attribute based on loads

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D64258#1572056 <https://reviews.llvm.org/D64258#1572056>, @jdoerfert wrote:

> Thanks for thinking of me ;) And again, I think this is an important change we need!
>
> The Attributor is in tree and, if enabled, it is run very early (as I very very strongly believe it should). I think we can get the Attributor enabled for the next release (maybe with a low iteration count and restrictions on the attributes we derive). Now there are two missing parts to get this functionality into the Attributor in a decent way:
>
> 1. A generic way to "look around for existing information" (more on this below).
> 2. The abstract attribute for dereferenceability(_or_null) that makes use of 1) and potentially performs usual deduction.
>
>   Implementing 2) is fairly easy. It should not take long to create the boilerplate if we only want to rely on the deduction through 1). Also, the logic is already in this patch (and the old prototype). Regarding 1): I was going to work on this once I found some free cycles but I could do it now if we decide to go this way. The idea is that you specify a program point PP (=instruction) and a callback. The callback is then automatically applied to all instruction which have to be executed when PP is also reached, either before or after. I would like this to be an abstract interface from the get-go but I am also willing to provide the interface and the initial implementation that will at least suffice for this use case. It should then be used from the `AbstractAttribute::initialize` and `AbstractAttribute::updateImpl` method of the abstract attribute for the dereferenceable attribute (and others later as well).
>
> P.S. You should be aware of the change to dereferenceability that is going to happen very soon, see D61652 <https://reviews.llvm.org/D61652> and D63243 <https://reviews.llvm.org/D63243> (I'm still fixing that one).


Thanks for the links. I'm still trying to digest where we stand currently and weigh the timing/risk/effort.

Attributor is in trunk, but it is not enabled by default? Or there are no transforms implemented that run with the default opt pipeline?
The current branch date for clang 9 is July 18 (10 days from today). That seems very tight to implement this based on the patch reviews that I skimmed. Ie, there's still a lot of back-and-forth going on in those reviews.

IMO, this patch carries significant risk alone. I'm basing that on the fact that D27855 <https://reviews.llvm.org/D27855> still isn't enabled by default and the related D29999 <https://reviews.llvm.org/D29999> was reverted because it caused crashing (I still haven't tracked down why).
So -- unless it would mean significantly more work to have this in trunk and then ported to the new and better way -- it is less overall work/risk to proceed here as an intermediate step since I already created this patch.

If there's nothing majorly wrong here, we can get several days of bot/fuzz/real-world testing on this code before the release. We are going to deprecate InferFunctionAttrs and its existing transform as part of switching to Attributor anyway, right? I can mark this with 'FIXME' and try to help with the porting if I've assessed that correctly.


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

https://reviews.llvm.org/D64258