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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM with two requests inlined.

In D70714#1761254 <https://reviews.llvm.org/D70714#1761254>, @uenoku wrote:

> > 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 :)
>
> I can work on this. You mean that copy all test in `Transforms/FunctionAttrs/`, `Transforms/InferFunctionAttrs/` to some directory(something like `Transforms/Attributor`, right? What about ArgumentPromotion or something like that?


Yes, that is what I mean. Start with the ones that run the Attributor right now, or which should run the Attributor right now.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1842
+    DerefBytesState.takeKnownMaximum(KnownBytes);
+  }
+
----------------
uenoku wrote:
> jdoerfert wrote:
> > 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.
> ```
> int f(int *A){
>    *A = 0;
>    *(A+2) = 2;
>    *(A+1) = 1;
>    *(A+10) = 10;
> }
> ```
> In that case, AccessedBytesMap is `{0:4, 4:4, 8:4, 40:4}`.  AccessedBytesMap is std::map so it is iterated in accending order on key(Offset).
> So KnownBytes will be updated like this:
> |Access | KnownBytes
> |(0, 4)| 0 -> 4
> |(4, 4)| 4 -> 8
> |(8, 4)| 8 -> 12
> |(40, 4) | 12 (break)
This was too smart for me. Can you put this example as a comment into or above `computeKnownDerefBytesFromAccessedMap` please? I know I'll forget it again.


================
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);
+
----------------
uenoku wrote:
> jdoerfert wrote:
> > Why std::max? I thought the map holds `<offset, size>` entries.
> I want to handle these cases:
> ```
> void f(int *A){
>   double *B = (double*)A; 
>   *B = 0.0;
>   *A = 0;
> }
> void g(int *A){
>   double *B = (double*)A; 
>   *A = 0;
>   *B = 0.0;
> }
> ```
Makes sense! I think I was just tired and didn't read the code properly. Can you add this as a test case?


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

https://reviews.llvm.org/D70714