[InstCombine] Remove attributes after hoisting free above null check

[smeenai]

Summary

If the parameter had been annotated as nonnull because of the null
check, we want to remove the attribute, since it may no longer apply and
could result in miscompiles if left. Similarly, we also want to remove
undef-implying attributes, since they may not apply anymore either.

Fixes PR52110.

[nikic]

LGTM

This is not the most precise way to do it (I think it would be sufficient to drop nonnull and convert dereferenceable into dereferenceable_or_null), but I don't think there's particular value in being precise either.

[smeenai]

Adjust attributes more precisely

[smeenai]

Thanks for the review!

I decided to take the more precise approach, because it seemed cleaner and was a good excuse for me to learn more of the attribute APIs :) I agree about the preciseness being unnecessary, but it shouldn't hurt either.

I'm requesting review again because I've never worked with attributes before, so I wanted to double check that (a) this is what you had in mind, and (b) there's not a simpler way to do the attribute replacement (it's a bit annoying that CallBase supports addDereferenceableParamAttr directly but adding dereferenceable_or_null requires going through AttributeList, but it also seemed overkill to add that method for this single use case). Thanks!

[aqjune]

I thought about the validity of this transformation.
To be pedantic, the validity is unclear when ptr is
(1) an out-of-bounds pointer such that (intptr_t)ptr is 0.
But, I remember people wanted to regard free(ptr) as equivalent to free(null) in that case, so it would be fine.
(2) a partially undefined pointer s.t. ptr = undef | 1
But... I couldn't see any such expression appear in practice, and I don't think LLVM is strictly correct with respect to partially undef pointers

Therefore, if the transformation is considered beneficial, I think the optimization is fine.

[smeenai]

In D111515#3060167, @aqjune wrote:

I thought about the validity of this transformation.
To be pedantic, the validity is unclear when ptr is
(1) an out-of-bounds pointer such that (intptr_t)ptr is 0.
But, I remember people wanted to regard free(ptr) as equivalent to free(null) in that case, so it would be fine.
(2) a partially undefined pointer s.t. ptr = undef | 1
But... I couldn't see any such expression appear in practice, and I don't think LLVM is strictly correct with respect to partially undef pointers

Therefore, if the transformation is considered beneficial, I think the optimization is fine.

Thank you! Does the logic and implementation of this particular change (remove any non-null implying attributes when hoisting the call above the null check) make sense to you?

[nikic]

LGTM

[smeenai]

Add test for combination of nonnull and dereferenceable

[smeenai]

In D111515#3062371, @nikic wrote:

LGTM

Thanks for the quick reviews!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

static Instruction *tryToMoveFreeBeforeNullTest(CallInst &FI,
   // before TI.
   for (Instruction &Instr : llvm::make_early_inc_range(*FreeInstrBB)) {
     if (&Instr == FreeInstrBBTerminator)
       break;
     Instr.moveBefore(TI);
   }
   assert(FreeInstrBB->size() == 1 &&
          "Only the branch instruction should remain");
+
+  // Now that we've moved the call to free before the NULL check, we have to
+  // remove any attributes on its parameter that imply it's non-null, because
+  // those attributes might have only been valid because of the NULL check, and
+  // we can get miscompiles if we keep them. This is conservative if non-null is
+  // also implied by something other than the NULL check, but it's guaranteed to
+  // be correct, and the conservativeness won't matter in practice, since the
+  // attributes are irrelevant for the call to free itself and the pointer
+  // shouldn't be used after the call.
+  AttributeList Attrs = FI.getAttributes();
+  Attrs = Attrs.removeParamAttribute(FI.getContext(), 0, Attribute::NonNull);
+  Attribute Dereferenceable = Attrs.getParamAttr(0, Attribute::Dereferenceable);
+  if (Dereferenceable.isValid()) {
+    uint64_t Bytes = Dereferenceable.getDereferenceableBytes();
+    Attrs = Attrs.removeParamAttribute(FI.getContext(), 0,
+                                       Attribute::Dereferenceable);
+    Attrs = Attrs.addDereferenceableOrNullParamAttr(FI.getContext(), 0, Bytes);
+  }
+  FI.setAttributes(Attrs);
+
   return &FI;
 }
 
 Instruction *InstCombinerImpl::visitFree(CallInst &FI) {
   Value *Op = FI.getArgOperand(0);
 
   // free undef -> unreachable.
   if (isa<UndefValue>(Op)) {

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

if.then:                                          ; preds = %entry
   %bitcast = bitcast i32* %foo to i8*
   tail call void @free(i8* %bitcast)
   br label %if.end
 
 if.end:                                           ; preds = %entry, %if.then
   ret void
 }
 
+;; Test that nonnull-implying attributes on the parameter are adjusted when the
+;; call is moved, since they may no longer be valid and result in miscompiles if
+;; kept unchanged.
+define void @test_nonnull_free_move(i8* %foo) minsize {
+; CHECK-LABEL: @test_nonnull_free_move(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    tail call void @free(i8* [[FOO]])
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %tobool = icmp eq i8* %foo, null
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @free(i8* nonnull %foo)
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  ret void
+}
+
+define void @test_dereferenceable_free_move(i8* %foo) minsize {
+; CHECK-LABEL: @test_dereferenceable_free_move(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    tail call void @free(i8* dereferenceable_or_null(4) [[FOO]])
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %tobool = icmp eq i8* %foo, null
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @free(i8* dereferenceable(4) %foo)
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  ret void
+}
+
+define void @test_nonnull_dereferenceable_free_move(i8* %foo) minsize {
+; CHECK-LABEL: @test_nonnull_dereferenceable_free_move(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    tail call void @free(i8* dereferenceable_or_null(16) [[FOO]])
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %tobool = icmp eq i8* %foo, null
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @free(i8* nonnull dereferenceable(16) %foo)
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  ret void
+}
+
 ; The next four tests cover the semantics of the nofree attributes.  These
 ; are thought to be legal transforms, but an implementation thereof has
 ; been reverted once due to difficult to isolate fallout.
 
 ; TODO: Freeing a no-free pointer -> %foo must be null
 define void @test13(i8* nofree %foo) {
 ; CHECK-LABEL: @test13(
 ; CHECK-NEXT:    call void @free(i8* [[FOO:%.*]])