[PATCH] D53342: [SimplifyLibCalls] Mark known arguments with nonnull

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D53342#1306336 <https://reviews.llvm.org/D53342#1306336>, @xbolva00 wrote:

> And we would be even more powerful with FunctionAttrs as a Module pass  to use DT in arg attribute promotion logic (now we do it only for entry block).


Just for some context:
I'm working on attribute interference to improve all IPOs right now. 
In my experiments argument promotion is often limited by alias and dereferencability information.
Therefore I'd like this to happen so non-null information can be deduced by default which will
then improve dereferenceability information in the new interference pass.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:196
+  if (CI->getArgOperand(ArgNo)->getType()->getPointerAddressSpace())
+    return false;
+
----------------
xbolva00 wrote:
> jdoerfert wrote:
> > I don't get this check. Can you explain it to me (potentially in a comment)?
> In some AS, dereferencing null ptr may be fine. See discussion: https://reviews.llvm.org/D53666
> 
> Maybe this is not a proper check, well, suggestions welcome
You do this check already 3 lines above (` F->nullPointerIsDefined())`).


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:416
+    dropNonNullParam(CI, 0);
+    dropNonNullParam(CI, 1);
     return nullptr;
----------------
xbolva00 wrote:
> jdoerfert wrote:
> > It seems fragile to drop the non-null conditionally only. Shouldn't we always drop it and add it back if possible?
> Ideally it should be on Clang side. They had tried but the solutions were reverted after miscompiles.
That is not a good answer to my question. Doing it in clang seems reasonable, if that is not what we want/can do we can do it here but properly. Thus, I'd argue it should not depend on the length as otherwise, we get some weird behaviors that cause us to drop it if we normalized the length to a constant before but not if we didn't.


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

https://reviews.llvm.org/D53342