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

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 marked 2 inline comments as done.
xbolva00 added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:196
+  if (CI->getArgOperand(ArgNo)->getType()->getPointerAddressSpace())
+    return false;
+
----------------
jdoerfert wrote:
> 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())`).
Not enough. Then we have e.g.

"call void @llvm.memset.p5i8.i64(i8 addrspace(5)* nonnull align 8 {{.*}}, i8 0, i64 40, i1 false"

As noted by Eli, "null pointers are potentially valid in non-zero address-spaces"

(test/CodeGenOpenCL/amdgpu-nullptr.cl)


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:416
+    dropNonNullParam(CI, 0);
+    dropNonNullParam(CI, 1);
     return nullptr;
----------------
jdoerfert wrote:
> 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.
In some problematic C functions, yes, we should drop this attribute always for certain parameters.

...and here we would tag it properly when we are sure that the size is > 0... we should not do it e.g. for:

strcmp(a, b, n), since n = 0 is valid and we could miscompile code due to null check removal transformation.


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

https://reviews.llvm.org/D53342