[DSE] Check post-dominance for malloc+memset->calloc transform.

[asbirlea]

Summary

Aiming to address the regression discussed in
https://reviews.llvm.org/D103009.

[xbolva00]

Also some notes from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83022

implementing a check to see if the only condition between the malloc and memset is 'ptr != 0'.

+1. This is probably what we really want to make everybody happy.

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll

415

I would rather see better fix, so this motivating case still works (allow 'ptr compared with NULL'). Othewise I see no value from this optimization at all and it should be just removed from DSE.

I am somewhat scared that people talking about this pattern

memset(malloc(size), 0, size)

not suprised that why there are so many nasty bugs/exploits in C codebases.

[asbirlea]

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll

415

I would rather see better fix, so this motivating case still works (allow 'ptr compared with NULL'). Othewise I see no value from this optimization at all and it should be just removed from DSE.

Sure, revert is a viable option while looking to add the 'ptr compared with NULL' check, as that's not so straightforward.

[xbolva00]

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll

415

But probably we should just take your patch, to avoid regression as SLC's transformation was removed as DSE-based one was implemented.

[yurai007]

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll

415

'ptr compared with NULL' check, as that's not so straightforward.

Yes, it requires some dance with matching like here (old patch doing transformation in SLC): https://reviews.llvm.org/D101176

[alexfh]

Looks good to me!

[alexfh]

I think, it's the right first step, and if there's a need to make the transformation support more use cases like checking the result of malloc for null, that can be added as a separate step.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

if (StoredConstant && StoredConstant->isNullValue()) {
           auto *InnerCallee = Malloc->getCalledFunction();
           if (!InnerCallee)
             return false;
           LibFunc Func;
           if (!TLI.getLibFunc(*InnerCallee, Func) || !TLI.has(Func) ||
               Func != LibFunc_malloc)
             return false;
           if (Malloc->getOperand(0) == MemSet->getLength()) {
-            if (DT.dominates(Malloc, MemSet) &&
+            if (DT.dominates(Malloc, MemSet) && PDT.dominates(MemSet, Malloc) &&
                 memoryIsNotModifiedBetween(Malloc, MemSet, BatchAA, DL, &DT)) {
               IRBuilder<> IRB(Malloc);
               const auto &DL = Malloc->getModule()->getDataLayout();
               if (auto *Calloc =
                       emitCalloc(ConstantInt::get(IRB.getIntPtrTy(DL), 1),
                                  Malloc->getArgOperand(0), IRB, TLI)) {
                 MemorySSAUpdater Updater(&MSSA);
                 auto *LastDef = cast<MemoryDef>(

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll

 if.then:                                          ; preds = %entry
   tail call void @llvm.memset.p0i8.i64(i8* nonnull align 16 %call, i8 0, i64 %mul, i1 false)
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
   ret i8* %call
 }
 
+; FIXME: malloc+memset are not currently transformed into calloc unless the
+; memset post-dominates the malloc.
 define float* @pr25892(i64 %size) {
 ; CHECK-LABEL: @pr25892(
 ; CHECK:       entry:
-; CHECK-NEXT:    [[CALL:%.*]] = call i8* @calloc(i64 1, i64 [[SIZE:%.*]])
+; CHECK-NEXT:    [[CALL:%.*]] = call i8* @malloc(i64 [[SIZE:%.*]])
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[CALL]], null
 ; CHECK-NEXT:    br i1 [[CMP]], label [[CLEANUP:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    [[BC:%.*]] = bitcast i8* [[CALL]] to float*
+; CHECK-NEXT:    call void @llvm.memset.p0i8.i64(i8* %call, i8 0, i64 %size, i1 false)
 ; CHECK-NEXT:    br label [[CLEANUP]]
 ; CHECK:       cleanup:
 ; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi float* [ [[BC]], [[IF_END]] ], [ null, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    ret float* [[RETVAL_0]]
 ;
 entry:
   %call = call i8* @malloc(i64 %size) inaccessiblememonly
   %cmp = icmp eq i8* %call, null
   br i1 %cmp, label %cleanup, label %if.end
 if.end:
   %bc = bitcast i8* %call to float*
   call void @llvm.memset.p0i8.i64(i8* %call, i8 0, i64 %size, i1 false)
   br label %cleanup
 cleanup:
   %retval.0 = phi float* [ %bc, %if.end ], [ null, %entry ]
   ret float* %retval.0
 }
 
 define float* @pr25892_with_extra_store(i64 %size) {
 ; CHECK-LABEL: @pr25892_with_extra_store(
 ; CHECK:       entry:
-; CHECK-NEXT:    [[CALL:%.*]] = call i8* @calloc(i64 1, i64 [[SIZE:%.*]])
+; CHECK-NEXT:    [[CALL:%.*]] = call i8* @malloc(i64 [[SIZE:%.*]])
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[CALL]], null
 ; CHECK-NEXT:    br i1 [[CMP]], label [[CLEANUP:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    [[BC:%.*]] = bitcast i8* [[CALL]] to float*
+; CHECK-NEXT:    call void @llvm.memset.p0i8.i64(i8* %call, i8 0, i64 %size, i1 false)
+; CHECK-NEXT:    store i8 0, i8* %call, align 1
 ; CHECK-NEXT:    br label [[CLEANUP]]
 ; CHECK:       cleanup:
 ; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi float* [ [[BC]], [[IF_END]] ], [ null, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    ret float* [[RETVAL_0]]
 ;
 entry:
   %call = call i8* @malloc(i64 %size) inaccessiblememonly
   %cmp = icmp eq i8* %call, null