[SimplifyCFG] Update passingValueIsAlwaysUndefined to check more attributes

[aqjune]

Summary

This is a simple patch to update SimplifyCFG's passingValueIsAlwaysUndefined to inspect more attributes.

A new function CallBase::isPassingUndefUB checks attributes that imply noundef.

[xbolva00]

Can you add some tests with deref/derefornull attributes?

[aqjune]

Add tests

[aqjune]

@spatel Now I fully got what https://reviews.llvm.org/D96663#2578814 meant. This change is introducing assume because it introduces unreachable which finally turns into assume.

But, an ongoing clang frontend patch will introduce many more noundefs, and this will anyway create assumes;
If the assumes inserted by this patch cause problems, it is likely that the problems will happen with the noundef patch as well. I think it is about facing the problem earlier or not.

If we decide to be lightweight (and not to insert assume), I'll move towards SimplifyCFG to simply swap the phi's operand with undef if nonnull without noundef is met.

[spatel]

In D97244#2581118, @aqjune wrote:

@spatel Now I fully got what https://reviews.llvm.org/D96663#2578814 meant. This change is introducing assume because it introduces unreachable which finally turns into assume.

But, an ongoing clang frontend patch will introduce many more noundefs, and this will anyway create assumes;
If the assumes inserted by this patch cause problems, it is likely that the problems will happen with the noundef patch as well. I think it is about facing the problem earlier or not.

If we decide to be lightweight (and not to insert assume), I'll move towards SimplifyCFG to simply swap the phi's operand with undef if nonnull without noundef is met.

Another idea - proposal to limit the creation of assumes from simplifycfg:
D97306

[spatel]

LGTM. As with D61409, we want to watch for regressions, but I don't think we need to delay. If there are problems, we can try to use assume bundles or other means as suggested D97306.

[aqjune]

Okay, thanks..!

[Carrot]

llvm/include/llvm/IR/InstrTypes.h

1663

It is not true for "this" parameter. It always has dereferenceable and nonnull attribute, if "this" is not used in the function, passing in undef is no problem.

It causes wrong code generated for our application.

[aqjune]

llvm/include/llvm/IR/InstrTypes.h

1663

passing in undef is no problem

This property holds for nonnull and align, but not for the three attributes listed here. Passing undef to these is UB because LangRef says violation of these is UB. dereferenceable paragraph says:

The pointer should be well defined, otherwise it is undefined behavior

Is your program being transformed from passing valid this to passing undef by some pass? Then, the pass is the buggy one.

[Carrot]

llvm/include/llvm/IR/InstrTypes.h

1663

Since "this" is not used in the function, DeadArgumentElimination pass changes all callers' valid "this" to undef.

@spatel, what's your opinion on where to fix it?

[aqjune]

llvm/include/llvm/IR/InstrTypes.h

1663

Since "this" is not used in the function, DeadArgumentElimination pass changes all callers' valid "this" to undef.

Then DeadArgumentElimination should drop attributes like dereferenceable, etc from the argument definition I think, but reviewers may have different ideas about this.

llvm/include/llvm/IR/InstrTypes.h

public:
   /// Determine whether this argument is passed by value, in an alloca, or is
   /// preallocated.
   bool isPassPointeeByValueArgument(unsigned ArgNo) const {
     return paramHasAttr(ArgNo, Attribute::ByVal) ||
            paramHasAttr(ArgNo, Attribute::InAlloca) ||
            paramHasAttr(ArgNo, Attribute::Preallocated);
   }
 
+  /// Determine whether passing undef to this argument is undefined behavior.
+  /// If passing undef to this argument is UB, passing poison is UB as well
+  /// because poison is more undefined than undef.
+  bool isPassingUndefUB(unsigned ArgNo) const {
+    return paramHasAttr(ArgNo, Attribute::NoUndef) ||
+           // dereferenceable implies noundef.
+           paramHasAttr(ArgNo, Attribute::Dereferenceable) ||
+           // dereferenceable implies noundef, and null is a well-defined value.
+           paramHasAttr(ArgNo, Attribute::DereferenceableOrNull);
+  }
+
   /// Determine if there are is an inalloca argument. Only the last argument can
   /// have the inalloca attribute.
   bool hasInAllocaArgument() const {
     return !arg_empty() && paramHasAttr(arg_size() - 1, Attribute::InAlloca);
   }
 
   // FIXME: Once this API is no longer duplicated in `CallSite`, rename this to
   // better indicate that this may return a conservative answer.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

if (auto *CB = dyn_cast<CallBase>(Use)) {
       // A call to null is undefined.
       if (CB->getCalledOperand() == I)
         return true;
 
       if (C->isNullValue()) {
         for (const llvm::Use &Arg : CB->args())
           if (Arg == I) {
             unsigned ArgIdx = CB->getArgOperandNo(&Arg);
-            if (CB->paramHasAttr(ArgIdx, Attribute::NonNull) &&
-                CB->paramHasAttr(ArgIdx, Attribute::NoUndef)) {
+            if (CB->isPassingUndefUB(ArgIdx) &&
+                CB->paramHasAttr(ArgIdx, Attribute::NonNull)) {
               // Passing null to a nonnnull+noundef argument is undefined.
               return !PtrValueMayBeModified;
             }
           }
       } else if (isa<UndefValue>(C)) {
         // Passing undef to a noundef argument is undefined.
         for (const llvm::Use &Arg : CB->args())
           if (Arg == I) {
             unsigned ArgIdx = CB->getArgOperandNo(&Arg);
-            if (CB->paramHasAttr(ArgIdx, Attribute::NoUndef)) {
+            if (CB->isPassingUndefUB(ArgIdx)) {
               // Passing undef to a noundef argument is undefined.
               return true;
             }
           }
       }
     }
   }
   return false;

llvm/test/Transforms/SimplifyCFG/UnreachableEliminate.ll

else:
   call i8* @fn_nonnull_noundef_arg(i8* %phi)
   ret void
 }
 
 ; Optimizing this code should produce assume.
 define void @test9_deref(i1 %X, i8* %Y) {
 ; CHECK-LABEL: @test9_deref(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SPEC_SELECT:%.*]] = select i1 [[X:%.*]], i8* null, i8* [[Y:%.*]]
-; CHECK-NEXT:    [[TMP0:%.*]] = call i8* @fn_nonnull_deref_arg(i8* [[SPEC_SELECT]])
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i1 [[X:%.*]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP0]])
+; CHECK-NEXT:    [[TMP1:%.*]] = call i8* @fn_nonnull_deref_arg(i8* [[Y:%.*]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
   br i1 %X, label %if, label %else
 
 if:
   br label %else
 
 else:
   %phi = phi i8* [ %Y, %entry ], [ null, %if ]
   call i8* @fn_nonnull_deref_arg(i8* %phi)
   ret void
 }
 
 ; Optimizing this code should produce assume.
 define void @test9_deref_or_null(i1 %X, i8* %Y) {
 ; CHECK-LABEL: @test9_deref_or_null(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SPEC_SELECT:%.*]] = select i1 [[X:%.*]], i8* null, i8* [[Y:%.*]]
-; CHECK-NEXT:    [[TMP0:%.*]] = call i8* @fn_nonnull_deref_or_null_arg(i8* [[SPEC_SELECT]])
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i1 [[X:%.*]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP0]])
+; CHECK-NEXT:    [[TMP1:%.*]] = call i8* @fn_nonnull_deref_or_null_arg(i8* [[Y:%.*]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
   br i1 %X, label %if, label %else
 
 if:
   br label %else