[Attributes] Support int attributes with zero value

[nikic]

Summary

This regularly comes up as a stumbling stone when adding int attributes: They currently need to be encoded in a way to avoids the zero value.

This adds support for zero-value int attributes by a) making the ctor determine int/enum attribute based on attribute kind, not whether the value is non-zero and b) switching getRawIntAttr() to return an Optional, so that it's possible to distinguish a zero value from non-existence.

[nikic]

Fix profiling function.

[aeubanks]

one nit, otherwise lgtm

llvm/lib/IR/Attributes.cpp

102

seems dangerous to put assert in the else branch without parentheses, it might expand to nothing?

[serge-sans-paille]

llvm/lib/IR/Attributes.cpp

102

that's ok, the assert will never eat the semi column.

1659–1660

Based on the implementation of unpackVScaleRangeArgs I think you can early return None if the original Attr is None here

1661

I would be tempted to change the signature of thatone and the above to return an Optional instead of defaulting to zero, what do you think?

[nikic]

llvm/lib/IR/Attributes.cpp

1659–1660

I just realized that these methods aren't used at all, so I went ahead and just removed them entirely: https://github.com/llvm/llvm-project/commit/24b1340ff96434c272a2a4abd3e70609be577e8a

[nikic]

llvm/lib/IR/Attributes.cpp

1661

Done for getAllocSizeArgs() in https://github.com/llvm/llvm-project/commit/ac47db6acad29b6e077593430338be69d81cb6c0. I think getVScaleRangeMin() is fine as-is, because 0 is a sensible (and conservatively correct) minimum.

llvm/include/llvm/IR/Attributes.h

public:
   /// Return Attribute with the given Kind. The returned attribute will be
   /// invalid if the Kind is not present in the builder.
   Attribute getAttribute(Attribute::AttrKind Kind) const;
 
   /// Return Attribute with the given Kind. The returned attribute will be
   /// invalid if the Kind is not present in the builder.
   Attribute getAttribute(StringRef Kind) const;
 
-  /// Return raw (possibly packed/encoded) value of integer attribute or 0 if
+  /// Return raw (possibly packed/encoded) value of integer attribute or None if
   /// not set.
-  uint64_t getRawIntAttr(Attribute::AttrKind Kind) const;
+  Optional<uint64_t> getRawIntAttr(Attribute::AttrKind Kind) const;
 
   /// Retrieve the alignment attribute, if it exists.
   MaybeAlign getAlignment() const {
-    return MaybeAlign(getRawIntAttr(Attribute::Alignment));
+    return MaybeAlign(getRawIntAttr(Attribute::Alignment).value_or(0));
   }
 
   /// Retrieve the stack alignment attribute, if it exists.
   MaybeAlign getStackAlignment() const {
-    return MaybeAlign(getRawIntAttr(Attribute::StackAlignment));
+    return MaybeAlign(getRawIntAttr(Attribute::StackAlignment).value_or(0));
   }
 
   /// Retrieve the number of dereferenceable bytes, if the
   /// dereferenceable attribute exists (zero is returned otherwise).
   uint64_t getDereferenceableBytes() const {
-    return getRawIntAttr(Attribute::Dereferenceable);
+    return getRawIntAttr(Attribute::Dereferenceable).value_or(0);
   }
 
   /// Retrieve the number of dereferenceable_or_null bytes, if the
   /// dereferenceable_or_null attribute exists (zero is returned otherwise).
   uint64_t getDereferenceableOrNullBytes() const {
-    return getRawIntAttr(Attribute::DereferenceableOrNull);
+    return getRawIntAttr(Attribute::DereferenceableOrNull).value_or(0);
   }
 
   /// Retrieve type for the given type attribute.
   Type *getTypeAttr(Attribute::AttrKind Kind) const;
 
   /// Retrieve the byval type.
   Type *getByValType() const { return getTypeAttr(Attribute::ByVal); }
 

llvm/lib/IR/AttributeImpl.h

public:
 
   Type *getValueAsType() const;
 
   /// Used when sorting the attributes.
   bool operator<(const AttributeImpl &AI) const;
 
   void Profile(FoldingSetNodeID &ID) const {
     if (isEnumAttribute())
-      Profile(ID, getKindAsEnum(), static_cast<uint64_t>(0));
+      Profile(ID, getKindAsEnum());
     else if (isIntAttribute())
       Profile(ID, getKindAsEnum(), getValueAsInt());
     else if (isStringAttribute())
       Profile(ID, getKindAsString(), getValueAsString());
     else
       Profile(ID, getKindAsEnum(), getValueAsType());
   }
 
+  static void Profile(FoldingSetNodeID &ID, Attribute::AttrKind Kind) {
+    assert(Attribute::isEnumAttrKind(Kind) && "Expected enum attribute");
+    ID.AddInteger(Kind);
+  }
+
   static void Profile(FoldingSetNodeID &ID, Attribute::AttrKind Kind,
                       uint64_t Val) {
+    assert(Attribute::isIntAttrKind(Kind) && "Expected int attribute");
     ID.AddInteger(Kind);
-    if (Val) ID.AddInteger(Val);
+    ID.AddInteger(Val);
   }
 
   static void Profile(FoldingSetNodeID &ID, StringRef Kind, StringRef Values) {
     ID.AddString(Kind);
     if (!Values.empty()) ID.AddString(Values);
   }
 
   static void Profile(FoldingSetNodeID &ID, Attribute::AttrKind Kind,

llvm/lib/IR/Attributes.cpp

unpackVScaleRangeArgs(uint64_t Value) {
   unsigned MinValue = Value >> 32;
 
   return std::make_pair(MinValue,
                         MaxValue > 0 ? MaxValue : Optional<unsigned>());
 }
 
 Attribute Attribute::get(LLVMContext &Context, Attribute::AttrKind Kind,
                          uint64_t Val) {
-  if (Val)
-    assert(Attribute::isIntAttrKind(Kind) && "Not an int attribute");
-  else
-    assert(Attribute::isEnumAttrKind(Kind) && "Not an enum attribute");
+  bool IsIntAttr = Attribute::isIntAttrKind(Kind);
+  assert((IsIntAttr || Attribute::isEnumAttrKind(Kind)) &&
+         "Not an enum or int attribute");
 
   LLVMContextImpl *pImpl = Context.pImpl;
   FoldingSetNodeID ID;
   ID.AddInteger(Kind);
-  if (Val) ID.AddInteger(Val);
+  if (IsIntAttr)
+    ID.AddInteger(Val);
+  else
+    assert(Val == 0 && "Value must be zero for enum attributes");
 
   void *InsertPoint;
   AttributeImpl *PA = pImpl->AttrsSet.FindNodeOrInsertPos(ID, InsertPoint);
 
   if (!PA) {
     // If we didn't find any existing attributes of the same shape then create a
     // new one and insert it.
-    if (!Val)
+    if (!IsIntAttr)
       PA = new (pImpl->Alloc) EnumAttributeImpl(Kind);
     else
       PA = new (pImpl->Alloc) IntAttributeImpl(Kind, Val);
     pImpl->AttrsSet.InsertNode(PA, InsertPoint);
   }
 
   // Return the Attribute that we found or created.
   return Attribute(PA);

 
 AttrBuilder &AttrBuilder::removeAttribute(StringRef A) {
   auto It = lower_bound(Attrs, A, AttributeComparator());
   if (It != Attrs.end() && It->hasAttribute(A))
     Attrs.erase(It);
   return *this;
 }
 
-uint64_t AttrBuilder::getRawIntAttr(Attribute::AttrKind Kind) const {
+Optional<uint64_t> AttrBuilder::getRawIntAttr(Attribute::AttrKind Kind) const {
   assert(Attribute::isIntAttrKind(Kind) && "Not an int attribute");
   Attribute A = getAttribute(Kind);
-  return A.isValid() ? A.getValueAsInt() : 0;
+  if (A.isValid())
+    return A.getValueAsInt();
+  return None;
 }
 
 AttrBuilder &AttrBuilder::addRawIntAttr(Attribute::AttrKind Kind,
                                         uint64_t Value) {
   return addAttribute(Attribute::get(Ctx, Kind, Value));
 }
 
 std::pair<unsigned, Optional<unsigned>> AttrBuilder::getAllocSizeArgs() const {
-  return unpackAllocSizeArgs(getRawIntAttr(Attribute::AllocSize));
+  return unpackAllocSizeArgs(getRawIntAttr(Attribute::AllocSize).value_or(0));
 }
 
 AttrBuilder &AttrBuilder::addAlignmentAttr(MaybeAlign Align) {
   if (!Align)
     return *this;
 
   assert(*Align <= llvm::Value::MaximumAlignment && "Alignment too large.");
   return addRawIntAttr(Attribute::Alignment, Align->value());
 }
 
 AttrBuilder &AttrBuilder::addStackAlignmentAttr(MaybeAlign Align) {
   // Default alignment, allow the target to define how to align it.