[IR] Convert null-pointer-is-valid into an enum attribute

[nikic]

Summary

The "null-pointer-is-valid" attribute needs to be checked by many pointer-related combines. To make the check more efficient, convert it from a string into an enum attribute.

While D78859 already makes the check for the attribute cheaper, this is another 0.4% improvement.

[xbolva00]

Just wondering if this allso enables

attribute((null_pointer_is_valid)) in Clang?

[nikic]

@xbolva00 This should not change any front-end behavior. I believe in clang this is controlled by -fno-delete-null-pointer-checks.

[nikic]

Update tests

[arsenm]

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

[arsenm]

In D78862#2003684, @arsenm wrote:

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

Also would be address space specific

[nikic]

In D78862#2003684, @arsenm wrote:

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

Is this change planned more for the near term or the long term? I'm not really familiar with ongoing addrspace related work.

[nikic]

@arsenm If the null-pointer-is-valid attribute is moved into the data layout, I'm wondering how Clang's -fno-delete-null-pointer-checks option would work or what it would be replaced with. In Rust it is possible to define a custom target, which also defines a custom data-layout, though I think that also needs to be compatible with the base target. I couldn't find any information on how to use a custom data layout in Clang. As such, I suspect that to preserve existing functionality we'd need support both for specifying this per-addrspace in the DL and as a function attribute.

[manojgupta]

@nikic Thanks for the work.

In D78862#2003684, @arsenm wrote:

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

@arsenm Is there any work planned on moving to data layout? Moving to data layout may affect cross TU inlining e.g. LTO where 1 TU is compiled with -fno-delete-null-pointer-checks and other TU is not. There might be other potential impact that we might not know yet.

[jdoerfert]

I think this is an improvement over the status quo and it looks fine to me.

@arsenm I agree that we should tie this to the data layout (or at least should try) but I guess there are open questions to answer and code to write.
I propose to accept this and work on the DL patch after. WDYT?

In D78862#2008831, @manojgupta wrote:

@nikic Thanks for the work.

In D78862#2003684, @arsenm wrote:

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

@arsenm Is there any work planned on moving to data layout? Moving to data layout may affect cross TU inlining e.g. LTO where 1 TU is compiled with -fno-delete-null-pointer-checks and other TU is not. There might be other potential impact that we might not know yet.

If we link modules with mismatching data layouts we can/should deal with this by utilizing more address spaces. That is, change the address space in one module to a fresh one to keep the properties alive. There need to be rules for this and infrastructure but something similar might be needed for heterogeneous IR modules soon. Different story though. We can also combine the attribute and the data layout if necessary, though I'm not a fan.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1321 ↗

(On Diff #260105)

I guess this change can go in as NFC simplification right away.

[nikic]

Rebase over committed NFC changes.

[nikic]

Fix rebase mistake in test

[nikic]

Ping

[arsenm]

In D78862#2012560, @jdoerfert wrote:

I think this is an improvement over the status quo and it looks fine to me.

@arsenm I agree that we should tie this to the data layout (or at least should try) but I guess there are open questions to answer and code to write.
I propose to accept this and work on the DL patch after. WDYT?

Seems ok, but it's still burning an enum value which I guess isn't super important. With the datalayout property, we might really want the inverse attribute

In D78862#2008831, @manojgupta wrote:

@nikic Thanks for the work.

In D78862#2003684, @arsenm wrote:

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

@arsenm Is there any work planned on moving to data layout? Moving to data layout may affect cross TU inlining e.g. LTO where 1 TU is compiled with -fno-delete-null-pointer-checks and other TU is not. There might be other potential impact that we might not know yet.

If we link modules with mismatching data layouts we can/should deal with this by utilizing more address spaces. That is, change the address space in one module to a fresh one to keep the properties alive. There need to be rules for this and infrastructure but something similar might be needed for heterogeneous IR modules soon. Different story though. We can also combine the attribute and the data layout if necessary, though I'm not a fan.

This sounds really problematic and would require way more knowledge of target address spaces. I don't think this will work

[jdoerfert]

In D78862#2027254, @arsenm wrote:

In D78862#2012560, @jdoerfert wrote:

I think this is an improvement over the status quo and it looks fine to me.

@arsenm I agree that we should tie this to the data layout (or at least should try) but I guess there are open questions to answer and code to write.
I propose to accept this and work on the DL patch after. WDYT?

Seems ok, but it's still burning an enum value which I guess isn't super important. With the datalayout property, we might really want the inverse attribute

Given that we lifted the limits on enum attributes I don't think this cost is high. Since we do not have a scheduled alternative, I think this should land.

docs/LangRef.rst

``nosync``
     the behavior is undefined.
 ``nounwind``
     This function attribute indicates that the function never raises an
     exception. If the function does raise an exception, its runtime
     behavior is undefined. However, functions marked nounwind may still
     trap or generate asynchronous exceptions. Exception handling schemes
     that are recognized by LLVM to handle asynchronous exceptions, such
     as SEH, will still provide their implementation defined semantics.
-``"null-pointer-is-valid"``
-   If ``"null-pointer-is-valid"`` is set to ``"true"``, then ``null`` address
+``null_pointer_is_valid``
+   If ``null_pointer_is_valid`` is set, then the ``null`` address
    in address-space 0 is considered to be a valid address for memory loads and
    stores. Any analysis or optimization should not treat dereferencing a
    pointer to ``null`` as undefined behavior in this function.
    Note: Comparing address of a global variable to ``null`` may still
    evaluate to false because of a limitation in querying this attribute inside
    constant expressions.
 ``optforfuzzing``
     This attribute indicates that this function should be optimized

include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_OPT_FOR_FUZZING = 57,
   ATTR_KIND_SHADOWCALLSTACK = 58,
   ATTR_KIND_SPECULATIVE_LOAD_HARDENING = 59,
   ATTR_KIND_IMMARG = 60,
   ATTR_KIND_WILLRETURN = 61,
   ATTR_KIND_NOFREE = 62,
   ATTR_KIND_NOSYNC = 63,
   ATTR_KIND_SANITIZE_MEMTAG = 64,
+  ATTR_KIND_NULL_POINTER_IS_VALID = 65,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

include/llvm/IR/Attributes.td

 def NoSync : EnumAttr<"nosync">;
 
 /// Disable Indirect Branch Tracking.
 def NoCfCheck : EnumAttr<"nocf_check">;
 
 /// Function doesn't unwind stack.
 def NoUnwind : EnumAttr<"nounwind">;
 
+/// Null pointer in address space zero is valid.
+def NullPointerIsValid : EnumAttr<"null_pointer_is_valid">;
+
 /// Select optimizations for best fuzzing signal.
 def OptForFuzzing : EnumAttr<"optforfuzzing">;
 
 /// opt_size.
 def OptimizeForSize : EnumAttr<"optsize">;
 
 /// Function must not be optimized.
 def OptimizeNone : EnumAttr<"optnone">;

include/llvm/IR/AutoUpgrade.h

namespace llvm {
 
   /// Upgrade the loop attachment metadata node.
   MDNode *upgradeInstructionLoopAttachment(MDNode &N);
 
   /// Upgrade the datalayout string by adding a section for address space
   /// pointers.
   std::string UpgradeDataLayoutString(StringRef DL, StringRef Triple);
 
-  /// Upgrade function attributes "no-frame-pointer-elim" and
-  /// "no-frame-pointer-elim-non-leaf" to "frame-pointer".
-  void UpgradeFramePointerAttributes(AttrBuilder &B);
+  /// Upgrade attributes that changed format or kind.
+  void UpgradeAttributes(AttrBuilder &B);
 
 } // End llvm namespace
 
 #endif

lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(norecurse);
   KEYWORD(nonlazybind);
   KEYWORD(nonnull);
   KEYWORD(noredzone);
   KEYWORD(noreturn);
   KEYWORD(nosync);
   KEYWORD(nocf_check);
   KEYWORD(nounwind);
+  KEYWORD(null_pointer_is_valid);
   KEYWORD(optforfuzzing);
   KEYWORD(optnone);
   KEYWORD(optsize);
   KEYWORD(readnone);
   KEYWORD(readonly);
   KEYWORD(returned);
   KEYWORD(returns_twice);
   KEYWORD(signext);

lib/AsmParser/LLParser.cpp

while (true) {
     case lltok::kw_noinline: B.addAttribute(Attribute::NoInline); break;
     case lltok::kw_nonlazybind: B.addAttribute(Attribute::NonLazyBind); break;
     case lltok::kw_noredzone: B.addAttribute(Attribute::NoRedZone); break;
     case lltok::kw_noreturn: B.addAttribute(Attribute::NoReturn); break;
     case lltok::kw_nosync: B.addAttribute(Attribute::NoSync); break;
     case lltok::kw_nocf_check: B.addAttribute(Attribute::NoCfCheck); break;
     case lltok::kw_norecurse: B.addAttribute(Attribute::NoRecurse); break;
     case lltok::kw_nounwind: B.addAttribute(Attribute::NoUnwind); break;
+    case lltok::kw_null_pointer_is_valid:
+      B.addAttribute(Attribute::NullPointerIsValid); break;
     case lltok::kw_optforfuzzing:
       B.addAttribute(Attribute::OptForFuzzing); break;
     case lltok::kw_optnone: B.addAttribute(Attribute::OptimizeNone); break;
     case lltok::kw_optsize: B.addAttribute(Attribute::OptimizeForSize); break;
     case lltok::kw_readnone: B.addAttribute(Attribute::ReadNone); break;
     case lltok::kw_readonly: B.addAttribute(Attribute::ReadOnly); break;
     case lltok::kw_returns_twice:
       B.addAttribute(Attribute::ReturnsTwice); break;

lib/AsmParser/LLToken.h

enum Kind {
   kw_norecurse,
   kw_nonlazybind,
   kw_nonnull,
   kw_noredzone,
   kw_noreturn,
   kw_nosync,
   kw_nocf_check,
   kw_nounwind,
+  kw_null_pointer_is_valid,
   kw_optforfuzzing,
   kw_optnone,
   kw_optsize,
   kw_readnone,
   kw_readonly,
   kw_returned,
   kw_returns_twice,
   kw_signext,

lib/Bitcode/Reader/BitcodeReader.cpp

case Attribute::ArgMemOnly:
     llvm_unreachable("argmemonly attribute not supported in raw format");
     break;
   case Attribute::AllocSize:
     llvm_unreachable("allocsize not supported in raw format");
     break;
   case Attribute::SanitizeMemTag:
     llvm_unreachable("sanitize_memtag attribute not supported in raw format");
     break;
+  case Attribute::NullPointerIsValid:
+    llvm_unreachable(
+        "null_pointer_is_valid attribute not supported in raw format");
+    break;
   }
   llvm_unreachable("Unsupported attribute type");
 }
 
 static void addRawAttributeValue(AttrBuilder &B, uint64_t Val) {
   if (!Val) return;
 
   for (Attribute::AttrKind I = Attribute::None; I != Attribute::EndAttrKinds;
        I = Attribute::AttrKind(I + 1)) {
-    if (I == Attribute::SanitizeMemTag ||
+    if (I == Attribute::NullPointerIsValid ||
+        I == Attribute::SanitizeMemTag ||
         I == Attribute::Dereferenceable ||
         I == Attribute::DereferenceableOrNull ||
         I == Attribute::ArgMemOnly ||
         I == Attribute::AllocSize ||
         I == Attribute::NoSync)
       continue;
     if (uint64_t A = (Val & getRawAttributeMask(I))) {
       if (I == Attribute::Alignment)
static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_NO_RETURN:
     return Attribute::NoReturn;
   case bitc::ATTR_KIND_NOSYNC:
     return Attribute::NoSync;
   case bitc::ATTR_KIND_NOCF_CHECK:
     return Attribute::NoCfCheck;
   case bitc::ATTR_KIND_NO_UNWIND:
     return Attribute::NoUnwind;
+  case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
+    return Attribute::NullPointerIsValid;
   case bitc::ATTR_KIND_OPT_FOR_FUZZING:
     return Attribute::OptForFuzzing;
   case bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE:
     return Attribute::OptimizeForSize;
   case bitc::ATTR_KIND_OPTIMIZE_NONE:
     return Attribute::OptimizeNone;
   case bitc::ATTR_KIND_READ_NONE:
     return Attribute::ReadNone;
case bitc::PARAMATTR_GRP_CODE_ENTRY: { // ENTRY: [grpid, idx, a0, a1, ...]
           Attribute::AttrKind Kind;
           if (Error Err = parseAttrKind(Record[++i], &Kind))
             return Err;
           if (Kind == Attribute::ByVal)
             B.addByValAttr(HasType ? getTypeByID(Record[++i]) : nullptr);
         }
       }
 
-      UpgradeFramePointerAttributes(B);
+      UpgradeAttributes(B);
       MAttributeGroups[GrpID] = AttributeList::get(Context, Idx, B);
       break;
     }
     }
   }
 }
 
 Error BitcodeReader::parseTypeTable() {

lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::NoReturn:
     return bitc::ATTR_KIND_NO_RETURN;
   case Attribute::NoSync:
     return bitc::ATTR_KIND_NOSYNC;
   case Attribute::NoCfCheck:
     return bitc::ATTR_KIND_NOCF_CHECK;
   case Attribute::NoUnwind:
     return bitc::ATTR_KIND_NO_UNWIND;
+  case Attribute::NullPointerIsValid:
+    return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;
   case Attribute::OptForFuzzing:
     return bitc::ATTR_KIND_OPT_FOR_FUZZING;
   case Attribute::OptimizeForSize:
     return bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE;
   case Attribute::OptimizeNone:
     return bitc::ATTR_KIND_OPTIMIZE_NONE;
   case Attribute::ReadNone:
     return bitc::ATTR_KIND_READ_NONE;

lib/CodeGen/CGCall.cpp

case CodeGenOptions::FramePointerKind::All:
       break;
     }
     FuncAttrs.addAttribute("frame-pointer", FpKind);
 
     FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
     if (CodeGenOpts.NullPointerIsValid)
-      FuncAttrs.addAttribute("null-pointer-is-valid", "true");
+      FuncAttrs.addAttribute(llvm::Attribute::NullPointerIsValid);
 
     if (CodeGenOpts.FPDenormalMode != llvm::DenormalMode::getIEEE())
       FuncAttrs.addAttribute("denormal-fp-math",
                              CodeGenOpts.FPDenormalMode.str());
     if (CodeGenOpts.FP32DenormalMode != CodeGenOpts.FPDenormalMode) {
       FuncAttrs.addAttribute(
           "denormal-fp-math-f32",
           CodeGenOpts.FP32DenormalMode.str());

lib/IR/Attributes.cpp

std::string Attribute::getAsString(bool InAttrGrp) const {
   if (hasAttribute(Attribute::NonNull))
     return "nonnull";
   if (hasAttribute(Attribute::NoRedZone))
     return "noredzone";
   if (hasAttribute(Attribute::NoReturn))
     return "noreturn";
   if (hasAttribute(Attribute::NoSync))
     return "nosync";
+  if (hasAttribute(Attribute::NullPointerIsValid))
+    return "null_pointer_is_valid";
   if (hasAttribute(Attribute::WillReturn))
     return "willreturn";
   if (hasAttribute(Attribute::NoCfCheck))
     return "nocf_check";
   if (hasAttribute(Attribute::NoRecurse))
     return "norecurse";
   if (hasAttribute(Attribute::NoUnwind))
     return "nounwind";
if (Caller.hasFnAttribute("min-legal-vector-width")) {
     } else {
       // If the callee doesn't have the attribute then we don't know anything
       // and must drop the attribute from the caller.
       Caller.removeFnAttr("min-legal-vector-width");
     }
   }
 }
 
-/// If the inlined function has "null-pointer-is-valid=true" attribute,
+/// If the inlined function has null_pointer_is_defined attribute,
 /// set this attribute in the caller post inlining.
 static void
 adjustNullPointerValidAttr(Function &Caller, const Function &Callee) {
   if (Callee.nullPointerIsDefined() && !Caller.nullPointerIsDefined()) {
-    Caller.addFnAttr(Callee.getFnAttribute("null-pointer-is-valid"));
+    Caller.addFnAttr(Attribute::NullPointerIsValid);
   }
 }
 
 struct EnumAttr {
   static bool isSet(const Function &Fn,
                     Attribute::AttrKind Kind) {
     return Fn.hasFnAttribute(Kind);
   }

lib/IR/AutoUpgrade.cpp

std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
   if (!R.match(DL, &Groups))
     return std::string(DL);
 
   SmallString<1024> Buf;
   std::string Res = (Groups[1] + AddrSpaces + Groups[3]).toStringRef(Buf).str();
   return Res;
 }
 
-void llvm::UpgradeFramePointerAttributes(AttrBuilder &B) {
+void llvm::UpgradeAttributes(AttrBuilder &B) {
   StringRef FramePointer;
   if (B.contains("no-frame-pointer-elim")) {
     // The value can be "true" or "false".
     for (const auto &I : B.td_attrs())
       if (I.first == "no-frame-pointer-elim")
         FramePointer = I.second == "true" ? "all" : "none";
     B.removeAttribute("no-frame-pointer-elim");
   }
   if (B.contains("no-frame-pointer-elim-non-leaf")) {
     // The value is ignored. "no-frame-pointer-elim"="true" takes priority.
     if (FramePointer != "all")
       FramePointer = "non-leaf";
     B.removeAttribute("no-frame-pointer-elim-non-leaf");
   }
-
   if (!FramePointer.empty())
     B.addAttribute("frame-pointer", FramePointer);
+
+  if (B.contains("null-pointer-is-valid")) {
+    // The value can be "true" or "false".
+    bool NullPointerIsValid = false;
+    for (const auto &I : B.td_attrs())
+      if (I.first == "null-pointer-is-valid")
+        NullPointerIsValid = I.second == "true";
+    B.removeAttribute("null-pointer-is-valid");
+    if (NullPointerIsValid)
+      B.addAttribute(Attribute::NullPointerIsValid);
+  }
 }

lib/IR/Function.cpp

assert(cast<MDString>(MD->getOperand(0))
                .equals("function_section_prefix") &&
            "Metadata not match");
     return cast<MDString>(MD->getOperand(1))->getString();
   }
   return None;
 }
 
 bool Function::nullPointerIsDefined() const {
-  return getFnAttribute("null-pointer-is-valid")
-          .getValueAsString()
-          .equals("true");
+  return hasFnAttribute(Attribute::NullPointerIsValid);
 }
 
 bool llvm::NullPointerIsDefined(const Function *F, unsigned AS) {
   if (F && F->nullPointerIsDefined())
     return true;
 
   if (AS != 0)
     return true;
 
   return false;
 }

lib/IR/Verifier.cpp

static bool isFuncOnlyAttr(Attribute::AttrKind Kind) {
   case Attribute::ArgMemOnly:
   case Attribute::NoRecurse:
   case Attribute::InaccessibleMemOnly:
   case Attribute::InaccessibleMemOrArgMemOnly:
   case Attribute::AllocSize:
   case Attribute::SpeculativeLoadHardening:
   case Attribute::Speculatable:
   case Attribute::StrictFP:
+  case Attribute::NullPointerIsValid:
     return true;
   default:
     break;
   }
   return false;
 }
 
 /// Return true if this is a function attribute that can also appear on

lib/Transforms/Utils/CodeExtractor.cpp

if (Attr.isStringAttribute()) {
       case Attribute::MinSize:
       case Attribute::NoDuplicate:
       case Attribute::NoFree:
       case Attribute::NoImplicitFloat:
       case Attribute::NoInline:
       case Attribute::NonLazyBind:
       case Attribute::NoRedZone:
       case Attribute::NoUnwind:
+      case Attribute::NullPointerIsValid:
       case Attribute::OptForFuzzing:
       case Attribute::OptimizeNone:
       case Attribute::OptimizeForSize:
       case Attribute::SafeStack:
       case Attribute::ShadowCallStack:
       case Attribute::SanitizeAddress:
       case Attribute::SanitizeMemory:
       case Attribute::SanitizeThread:

test/Bitcode/attributes.ll

 }
 
 ; CHECK: define void @f63() #39
 define void @f63() sanitize_memtag
 {
   ret void;
 }
 
+; CHECK: define void @f64() #40
+define void @f64() null_pointer_is_valid
+{
+  ret void;
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { readnone }
 ; CHECK: attributes #3 = { readonly }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

 ; CHECK: attributes #32 = { writeonly }
 ; CHECK: attributes #33 = { speculatable }
 ; CHECK: attributes #34 = { sanitize_hwaddress }
 ; CHECK: attributes #35 = { shadowcallstack }
 ; CHECK: attributes #36 = { willreturn }
 ; CHECK: attributes #37 = { nofree }
 ; CHECK: attributes #38 = { nosync }
 ; CHECK: attributes #39 = { sanitize_memtag }
-; CHECK: attributes #40 = { nobuiltin }
+; CHECK: attributes #40 = { null_pointer_is_valid }
+; CHECK: attributes #41 = { nobuiltin }

test/CodeGen/delete-null-pointer-checks.c

 // NULL-POINTER-VALID: load i32, i32* %[[SEL:.*]]
   int *Q = P;
   if (P) {
     Q = P + 2;
   }
   return *Q;
 }
 
-// NULL-POINTER-INVALID-NOT: attributes #0 = {{.*}} "null-pointer-is-valid"="true"
-// NULL-POINTER-VALID: attributes #0 = {{.*}} "null-pointer-is-valid"="true"
+// NULL-POINTER-INVALID-NOT: attributes #0 = {{.*}} null_pointer_is_valid
+// NULL-POINTER-VALID: attributes #0 = {{.*}} null_pointer_is_valid