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

Thu Apr 21 11:36:50 PDT 2022

anna added a comment.

In D124115#3462940 <https://reviews.llvm.org/D124115#3462940>, @nikic wrote:

> In D124115#3462890 <https://reviews.llvm.org/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".

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124115/new/

https://reviews.llvm.org/D124115