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

Hideto Ueno via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 26 23:29:09 PST 2019


uenoku marked 5 inline comments as done.
uenoku added a comment.

> 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?



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1842
+    DerefBytesState.takeKnownMaximum(KnownBytes);
+  }
+
----------------
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)


================
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);
+
----------------
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;
}
```


================
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) {
----------------
jdoerfert wrote:
> 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.
Yes. I'll commit separately.


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

https://reviews.llvm.org/D70714





More information about the llvm-commits mailing list