[Attributes] Update Attribute::get API to consider zero value for int attributes

[anna]

Summary

This patch allows Attribute::get API to consider zero for the uint64_t
argument passed in for int attributes.
Until now, the implicit assumption was that if the value is zero, then
it is an enum attribute. However, we can have cases (such a case exists
downstream for us), where an int attribute can have a zero value.

I initially tried Optional<uint64_t> instead of adding an extra default
argument. However there are places in Attributor where we pass in an enum
attribute with a zero value (see IRPosition::getAttrsFromAssumes). So the
alternate approach is recognize such cases and pass in None as the optional
argument (see D124116 for the idea).

[nikic]

I would prefer to split this into two overloads, one without the uint64_t argument creating an enum attribute and one with creating always an int attribute, and fix up any problematic uses accordingly. The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

[anna]

In D124115#3462855, @nikic wrote:

I would prefer to split this into two overloads, one without the uint64_t argument creating an enum attribute and one with creating always an int attribute, and fix up any problematic uses accordingly. The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

Actually, yes the overload is a better idea.

The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

I'm confused why we cannot have zero values for int attributes? The change here was to specifically allow zero values for any int attribute (because we have a downstream attribute where the value can be zero).
Is this a verifier issue - for example, we cannot have derferenceable_or_null with 0.

[nikic]

In D124115#3462890, @anna wrote:

The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

I'm confused why we cannot have zero values for int attributes? The change here was to specifically allow zero values for any int attribute (because we have a downstream attribute where the value can be zero).
Is this a verifier issue - for example, we cannot have derferenceable_or_null with 0.

I don't think there is a strong technical reason for this restriction, it's just an artifact of the API design. For some int attributes we perform an encode/decode step to translate between a "raw value" (which is never zero) and a semantic value (which can be zero). I do think we should lift this restriction, though I'm not sure if just adjusting this one constructor will be sufficient.

[anna]

In D124115#3462940, @nikic wrote:

In D124115#3462890, @anna wrote:

The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

I'm confused why we cannot have zero values for int attributes? The change here was to specifically allow zero values for any int attribute (because we have a downstream attribute where the value can be zero).
Is this a verifier issue - for example, we cannot have derferenceable_or_null with 0.

I don't think there is a strong technical reason for this restriction, it's just an artifact of the API design. For some int attributes we perform an encode/decode step to translate between a "raw value" (which is never zero) and a semantic value (which can be zero). I do think we should lift this restriction, though I'm not sure if just adjusting this one constructor will be sufficient.

Yes, it does look like just fixing this one constructor is not enough. For example, in the downstream case, using this change, we no longer fail on the assert about the kind of attribute when the attribute value is 0. However, I'm seeing another assertion failure: "Context of attribute is not the same as the module". I did not investigate the reason for this (but it is very clear that it only fails when the value is zero, because we also update other non-zero attributes with that same context and have no issues).
Also, when making sure the attribute will always have a non-zero value through our front-end, I no longer see that assertion failure. I'm going with the route of fixing the front-end for now until I get a chance to investigate the reason here on what else is preventing a "zero value int attribute".

[anna]

went another path.

llvm/include/llvm/IR/Attributes.h

 public:
   Attribute() = default;
 
   //===--------------------------------------------------------------------===//
   // Attribute Construction
   //===--------------------------------------------------------------------===//
 
   /// Return a uniquified Attribute object.
-  static Attribute get(LLVMContext &Context, AttrKind Kind, uint64_t Val = 0);
+  /// Val has a default value of zero which implies the AttrKind is enum.
+  /// If ConsiderZeroValueForIntAttr is true, then Val (even if zero) is
+  /// considered as the value for the attribute, which should be an Int
+  /// attribute.
+  static Attribute get(LLVMContext &Context, AttrKind Kind,
+                       uint64_t Val = 0, bool ConsiderZeroValueForIntAttr = false);
   static Attribute get(LLVMContext &Context, StringRef Kind,
                        StringRef Val = StringRef());
   static Attribute get(LLVMContext &Context, AttrKind Kind, Type *Ty);
 
   /// Return a uniquified Attribute object that has the specific
   /// alignment set.
   static Attribute getWithAlignment(LLVMContext &Context, Align Alignment);
   static Attribute getWithStackAlignment(LLVMContext &Context, Align Alignment);

llvm/lib/IR/Attributes.cpp

unpackVScaleRangeArgs(uint64_t Value) {
   unsigned MaxValue = Value & std::numeric_limits<unsigned>::max();
   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)
+                         uint64_t Val, bool ConsiderZeroValueForIntAttr) {
+  const bool isIntAttr = Val || ConsiderZeroValueForIntAttr;
+  if (isIntAttr)
     assert(Attribute::isIntAttrKind(Kind) && "Not an int attribute");
   else
     assert(Attribute::isEnumAttrKind(Kind) && "Not an enum attribute");
 
   LLVMContextImpl *pImpl = Context.pImpl;
   FoldingSetNodeID ID;
   ID.AddInteger(Kind);
-  if (Val) ID.AddInteger(Val);
+  if (isIntAttr) ID.AddInteger(Val);
 
   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);