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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:196
+  if (CI->getArgOperand(ArgNo)->getType()->getPointerAddressSpace())
+    return false;
+
----------------
xbolva00 wrote:
> 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)
This is very very cryptic. First, the explicitly named check and then some condition without explanation. Maybe use `NullPointerIsDefined` instead as it combines both checks and it's clear what is happening.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:416
+    dropNonNullParam(CI, 0);
+    dropNonNullParam(CI, 1);
     return nullptr;
----------------
xbolva00 wrote:
> 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.
> strcmp(a, b, n), since n = 0 is valid and we could miscompile code due to null check removal transformation.

Afaik, null pointers are never "valid" but often assumed to be valid. That said, I don't see why you want to drop non-null only conditionally. Your very argument is that for a lenght of 0 we should not have non-null, however, your patch actually keeps it in this and other cases for which != 0 was not shown.


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

https://reviews.llvm.org/D53342