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

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

xbolva00 marked an inline comment as done.
xbolva00 added a comment.

Found a bug in LLVM.

Intrinsic builder does not copy attributes, it seems.

define i8* @memset_size_select4(i1 %b, i8* %ptr) {
; CHECK-LABEL: @memset_size_select4(
; CHECK-NEXT:    [[SIZE:%.*]] = select i1 [[B:%.*]], i32 10, i32 50
; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* align 1 [[PTR:%.*]], i8 0, i32 [[SIZE]], i1 false)
; CHECK-NEXT:    ret i8* [[PTR]]
;

  %size = select i1 %b, i32 10, i32 50
  %memset = tail call i8* @memset(i8* nonnull dereferenceable_or_null(40) %ptr, i32 0, i32 %size)
  ret i8* %memset

}



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:2381
+  else
+    removeNonNull(CI, {0});
+
----------------
jdoerfert wrote:
> xbolva00 wrote:
> > jdoerfert wrote:
> > > We could even use "isKnownNonZero" or other helper functions here.
> > This is true but since we we need to compute “N” anyway, I think isKnownNonZero is a overkill a bit, since it computes what we already have - “N”. - should I use it even here?
> > 
> > But I could use it in some places, yes,  (memcpy/memmove/memset)..
> For a lot of calls a nonnull size implies other stuff, especially, noalias, deref_or_null.
> I think this is worth a separate patch but doing better nonnull checks makes sense because any value (not necessarily a constant) that is not zero will allow us to infer (1) nonnull, (2) noalias, (3) deref_or_null. If we want the constant size for some other transformations we should first look for a constant and only as a fallback call `isknownnonzero`.
Until I start heavy mechanical work here I would like to know if this approch is acceptable.

Value *LibCallSimplifier::optimizeMemSet(CallInst *CI, IRBuilder<> &B,
                                         bool isIntrinsic) {
  Value *Size = CI->getArgOperand(2);
  if (ConstantInt *LenC = dyn_cast<ConstantInt>(Size)) {
    annotateDereferenceableBytes(CI, {0}, LenC->getZExtValue());
    annotateNonNull(CI, {0});
  } else if (isKnownNonZero(Size, DL)) {
    annotateDereferenceableBytes(CI, {0}, 1);
    annotateNonNull(CI, {0});
  } else if (!isIntrinsic) {
    removeNonNull(CI, {0});
  }

Now we catch even this case :)
define i8* @memset_size_select(i1 %b, i8* %ptr) {
; CHECK-LABEL: @memset_size_select(
; CHECK-NEXT:    [[SIZE:%.*]] = select i1 [[B:%.*]], i32 10, i32 50
; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* nonnull align 1 dereferenceable(1) [[PTR:%.*]], i8 0, i32 [[SIZE]], i1 false)
; CHECK-NEXT:    ret i8* [[PTR]]
;
  %size = select i1 %b, i32 10, i32 50
  %memset = tail call i8* @memset(i8* nonnull %ptr, i32 0, i32 %size) #1
  ret i8* %memset
}


define i8* @memset_size_select_or(i1 %b, i8* %ptr, i32 %v) {
; CHECK-LABEL: @memset_size_select_or(
; CHECK-NEXT:    [[SIZE:%.*]] = ashr i32 -2, [[V:%.*]]
; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* nonnull align 1 dereferenceable(1) [[PTR:%.*]], i8 0, i32 [[SIZE]], i1 false)
; CHECK-NEXT:    ret i8* [[PTR]]
;
  %size = ashr i32 -2, %v
  %memset = tail call i8* @memset(i8* nonnull %ptr, i32 0, i32 %size) #1
  ret i8* %memset
}


for select of constants we can do better job than deref(1)

if (ConstantInt *LenC = dyn_cast<ConstantInt>(Size)) {
    annotateDereferenceableBytes(CI, {0}, LenC->getZExtValue());
    annotateNonNull(CI, {0});
  } else if (isKnownNonZero(Size, DL)) {
    const  APInt *X, *Y;
    uint64_t DerefMin = 1;
    if (match(Size, m_Select(m_Value(), m_APInt(X), m_APInt(Y)))) {
      Min = std::min(X->getZExtValue(), Y->getZExtValue());
          annotateDereferenceableBytes(CI, {0}, DerefMin);
}
    annotateNonNull(CI, {0});
  } else if (!isIntrinsic) {
    removeNonNull(CI, {0});
  }

Ok?


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

https://reviews.llvm.org/D53342