[InstCombine] Always remove null check before free

[xbolva00]

Summary

Null check before free is currently removed only when we are optimizing for size.

This patch removes that limitation since free already handles null pointer passed as argument so null check before free can be easily removed. One more useless branching at the end of functions to be removed.

[lebedev.ri]

1. Test?
2. Please always upload diffs with full context (-U99999)

[xbolva00]

Added test

[lebedev.ri]

I don't think you intended to add mypatch.patch

Also, i suspect ideally there should be an explanation of why "null check before free is currently removed only when we are optimizing for size." and then why "This patch removes that limitation."

test/Transforms/InstCombine/null-check-free.ll

1 ↗

(On Diff #141324)

Please use llvm/utils/update_test_checks.py.
Also, the test is incomplete, @nullcheckfree is only listed in the check line, but it's not defined anywhere,
(You need to upload the diff as compared to the git master/svn trunk, not as compared to the previous diff)

[lebedev.ri]

test/Transforms/InstCombine/null-check-free.ll

1 ↗

(On Diff #141324)

The usual comment at the top is missing though:

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py

[lebedev.ri]

Null check before free is currently removed only when we are optimizing for size.
This patch removes that limitation since free already handles null pointer passed as argument so null check before free can be easily removed.

I'm seeing "what", but not seeing "why"...
The original rL171762 talked about only doing that when it is known to be profitable.
It might be helpful to explain in the differential's description why it is now
preferable to always do this, not only when optimizing for size.

test/Transforms/InstCombine/null-check-free.ll

21 ↗

(On Diff #141487)

So what exactly does that diff change here in the output?
https://godbolt.org/g/CJaEsg i don't see any difference.

This is why it is best to have two separate differentials,
one with just the original testcase, and the other one with the code change + regenerated test output.

33 ↗

(On Diff #141487)

Hmm, i don't like the test. Does it not end up returning dangling pointer?

[xbolva00]

Patch updated with suggested changes by @lebedev.ri

[xbolva00]

ping

[spatel]

I don't understand what's going on here. Why is this patch marked 'Accepted'? In general, you shouldn't approve your own patches that you've posted for review.

Beyond that, I don't understand why this is desirable. I think the reason this was coded with size optimization as a predicate is because the null check cheaply guards against a potentially expensive library call (cc @qcolombet as author of the original patch). This might be the correct transform for instcombine, but then there should be a backend transform that would add the null check back if it's profitable?

[xbolva00]

In D45343#1069903, @spatel wrote:

I don't understand what's going on here. Why is this patch marked 'Accepted'? In general, you shouldn't approve your own patches that you've posted for review.

Beyond that, I don't understand why this is desirable. I think the reason this was coded with size optimization as a predicate is because the null check cheaply guards against a potentially expensive library call (cc @qcolombet as author of the original patch). This might be the correct transform for instcombine, but then there should be a backend transform that would add the null check back if it's profitable?

True. I will close this patch.

lib/Transforms/InstCombine/InstructionCombining.cpp

if (isa<UndefValue>(Op)) {
     return eraseInstFromFunction(FI);
   }
 
   // If we have 'free null' delete the instruction.  This can happen in stl code
   // when lots of inlining happens.
   if (isa<ConstantPointerNull>(Op))
     return eraseInstFromFunction(FI);
 
-  // If we optimize for code size, try to move the call to free before the null
-  // test so that simplify cfg can remove the empty block and dead code
-  // elimination the branch. I.e., helps to turn something like:
+  // Try to move the call to free before the null test so that simplify cfg can
+  // remove the empty block and dead code elimination the branch.
+  // I.e., helps to turn something like:
   // if (foo) free(foo);
   // into
   // free(foo);
-  if (MinimizeSize)
-    if (Instruction *I = tryToMoveFreeBeforeNullTest(FI))
-      return I;
+
 
   return nullptr;
 }
 
 Instruction *InstCombiner::visitReturnInst(ReturnInst &RI) {
   if (RI.getNumOperands() == 0) // ret void
     return nullptr;
 

test/Transforms/InstCombine/malloc-free-delete.ll

 
 ;; When a basic block contains only a call to free and this block is accessed
 ;; through a test of the argument of free against null, move the call in the
 ;; predecessor block.
 ;; Using simplifycfg will remove the empty basic block and the branch operation
 ;; Then, performing a dead elimination will remove the comparison.
 ;; This is what happens with -O1 and upper.
 ; CHECK-LABEL: @test6(
-define void @test6(i8* %foo) minsize {
+define void @test6(i8* %foo) {
 ; CHECK:  %tobool = icmp eq i8* %foo, null
 ;; Call to free moved
 ; CHECK-NEXT: tail call void @free(i8* %foo)
 ; CHECK-NEXT: br i1 %tobool, label %if.end, label %if.then
 ; CHECK: if.then:
 ;; Block is now empty and may be simplified by simplifycfg
 ; CHECK-NEXT:   br label %if.end
 ; CHECK: if.end: