[PATCH] D66079: [SimplifyLibCalls] Add dereferenceable bytes from known callsites [WIP]

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I like the direction, first obstacle identified but solvable. We need tests.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:193
+    unsigned ArgNo = Arg->getArgNo();
+    if (F->getParamDereferenceableBytes(ArgNo) < DerefBytes) {
+      F->removeParamAttr(ArgNo, Attribute::Dereferenceable);
----------------
xbolva00 wrote:
> Copy paste from @spatel 's patch: https://reviews.llvm.org/D64258
> 
> We should find proper place for this.
The Attributor has a similar, though more elaborate, utility which is not exposed yet. I would expect that to be usable but the interface is in flux right now. I'd go with this and a TODO note.

---

Edit: Reading through the use cases, maybe we should use the Attributor code which can handle more cases. What is happening here is not what we want, see my example below.

---

Proposal, do it here, sth like:

```
static void annotateDereferenceableBytes(CallSite CS, unsigned ArgNo, uint64_t DerefBytes) {
  if (CS.getDereferenceableBytes(ArgNo) < DerefBytes) {
    CS.removeParamAttr(ArgNo, Attribute::Dereferenceable);
    CS.removeParamAttr(ArgNo, Attribute::DereferenceableOrNull);
    // TODO: CallSite does not have an `addParamAttr` for integer attributes.
    CS.addAttribute(ArgNo + AttributeList::FirstArgIndex, Attribute::getWithDereferenceableBytes(CS->getContext(), DerefBytes));
  }
}
```

(I did not try to compile this!)


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:943
+    inferDereferenceableBytes(LHS, LenC->getZExtValue());
+    inferDereferenceableBytes(RHS, LenC->getZExtValue());
     if (Value *Res = optimizeMemCmpConstantSize(CI, LHS, RHS,
----------------
`LHS` and `RHS` are call site operands for which we deduced dereferenceability. Thus we want to annotate the call site. Now with this code, if I'm not mistaken, there are two possibilities for `LHS`, `RHS` is the same:

1) `LHS` is anything but an argument -> no annotation -> not what we want
2) `LHS` is an argument -> annotation at the argument -> not necessarily correct, see below:

We should not derive `deref` for the arguments `a` or `b` from the `memcmp` call. We can and should derive `deref` for their usage in the `memcmp` call.
```
void foo(int *a, int *b) {
  if (a && b)
    memcmp(a, b, 4);
}
```



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:983
+  if (isIntrinsic)
+    return nullptr;
+
----------------
What does the return value here mean? I'm also confused by the `Op0` which was there already.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D66079