[PATCH] D70714: [Attributor] Deduce dereferenceable based on accessed bytes map

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

After reading though the tests it looks to me like this works just fine. I still didn't quite understand why there is a std::max. Could you elaborate on that?

---

In D70714#1760190 <https://reviews.llvm.org/D70714#1760190>, @lebedev.ri wrote:

> In D70714#1760179 <https://reviews.llvm.org/D70714#1760179>, @spatel wrote:
>
> > There's also a question about whether this proposed Attributor functionality obsoletes: 
> >  D37579 <https://reviews.llvm.org/D37579>
>
>
> Not in it's present state, no.


Correct, I answered in the thread now as wellh.

> There's an upcoming RFC from @jdoerfert that will be aiming to improve that problem.

No pressure, ...

In D70714#1760177 <https://reviews.llvm.org/D70714#1760177>, @xbolva00 wrote:

> And I think Attributor needs to run before InstCombine and after InstCombine too. InstCombine infers a lot of derefereceable attrs for libcalls but there is no pass to propagate info futher into function signature, etc.


It needs to be (also) a CG-SCC pass. Working on that part right now.

In D70714#1760169 <https://reviews.llvm.org/D70714#1760169>, @spatel wrote:

> Please rebase/update with additional tests after: rG2bd252ea8941 <https://reviews.llvm.org/rG2bd252ea894189f77e09755cf6951727e1d03a74>


Yes. We have to figure out the test situation though. I think we need to copy all test used by the Attributor into a dedicted forlder. @uenoku @sstefan1 if one of you wants to start doing that, please let me know :)

> And what is the plan for enabling Attributor by default? Are there any known bugs?

Not at the time of the Dev meeting. The roadmap I made up right now:

- Make it an CG-SCC pass and run it in the inliner loop.
- Introduce a switch that determines what values we initially look at (the seeding phase) to limit compile time (which was not bad!).
- Redo my testing and fixing of new/exposed bugs.
- Send and RFC telling people to please run their tests with the Attributor.
- Enable it by default

Help with any of the steps is always appreciated, the first one I think I've got though.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1829
+  /// std::map is used because we want to iterate keys in ascending order.
+  std::map<int64_t, int64_t> AccessedBytesMap;
+
----------------
Could we do uint64_t?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1842
+    DerefBytesState.takeKnownMaximum(KnownBytes);
+  }
+
----------------
This confuses me. Do we handle "holes" correctly? Do we have a test for holes?

I mean

```
int *A = ...
A[ 0] = 2
A[10] = 1
```
should not result in deref(40) if the geps are not inbounds.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1885
+  void addAccessedBytes(int64_t Offset, int64_t Size) {
+    AccessedBytesMap[Offset] = std::max(AccessedBytesMap[Offset], Size);
+
----------------
Why std::max? I thought the map holds `<offset, size>` entries.


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:372
 
-; FXIME: %p should have nonnull
-; ATTRIBUTOR: define void @test12-4(i32* nocapture nofree writeonly align 16 %p)
+; ATTRIBUTOR: define void @test12-4(i32* nocapture nofree nonnull writeonly align 16 dereferenceable(8) %p)
 define void @test12-4(i32* align 4 %p) {
----------------
This is due to the special case with offset 0, correct? If so, commit that special case and these test changes on their own. LGTM on that part.


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:222
 ; CHECK-LABEL: @no_pointer_deref(i16* %ptr)
+; ATTRIBUTOR-LABEL: @no_pointer_deref(i16* nocapture nofree readonly %ptr)
   %arrayidx1 = getelementptr i16, i16* %ptr, i64 1
----------------
Here do the "hole" test start. I see.


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

https://reviews.llvm.org/D70714