[Mlir-commits] [llvm] [mlir] [IR][ModRef] Introduce `errno` memory location (PR #120783)

[Antonio Frighetto]

================
@@ -2293,9 +2318,21 @@ static bool upgradeOldMemoryAttribute(MemoryEffects &ME, uint64_t EncodedKind) {
   case bitc::ATTR_KIND_INACCESSIBLEMEM_ONLY:
     ME &= MemoryEffects::inaccessibleMemOnly();
     return true;
+  case bitc::ATTR_KIND_ERRNOMEMONLY:
+    ME &= MemoryEffects::errnoMemOnly();
+    return true;
   case bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEMONLY:
     ME &= MemoryEffects::inaccessibleOrArgMemOnly();
     return true;
+  case bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ERRNOMEMONLY:
+    ME &= MemoryEffects::inaccessibleOrErrnoMemOnly();
+    return true;
+  case bitc::ATTR_KIND_ARGMEM_OR_ERRNOMEMONLY:
+    ME &= MemoryEffects::argumentOrErrnoMemOnly();
+    return true;
+  case bitc::ATTR_KIND_INACCESSIBLEMEM_OR_ARGMEM_OR_ERRNOMEMONLY:
+    ME &= MemoryEffects::inaccessibleOrArgOrErrnoMemOnly();
+    return true;
----------------
antoniofrighetto wrote:

Thank you for clarifying this, I think it's a bit clearer now. I still have a doubt though: in `decodeLLVMAttributesForBitcode`, I believe I should expect to have something as follows:
```cpp
uint64_t Attrs = ((EncodedAttrs & (0xfffffULL << 32)) >> 11) | (EncodedAttrs & 0xffff);
uint8_t Version = (EncodedAttrs >> 56) & 0xFF;

if (AttrIdx == AttributeList::FunctionIndex) {
    // Upgrade old memory attributes.
    // ...
    // If version < 1 and attribute for Other is set, copy Other ModRef to Errno.
    if (Version < 1 && (Attrs & (7ULL << 39))
        ME &= MemoryEffects::errnoMemOnly(ME.getModRef(IRMemLocation::Other));

    if (ME != MemoryEffects::unknown())
      B.addMemoryAttr(ME);
}
```
However, we currently miss a mask for attribute Other to check if it was set, so this should be introduced too?

> A way to do that would be to add a version number to the high byte of the MemoryEffects bitcode encoding.

As per doc-comment in `decodeLLVMAttributesForBitcode`, I was expecting to see `encodeLLVMAttributesForBitcode` in the Writer, where to add a version number, but seems to have been dropped a while ago. Am I understanding correctly that in `writeAttributeTable` we should somehow still have something as follows?
```cpp
  const uint8_t Version = 1;
  uint64_t EncodedAttrs = (Version << 56) | ME.getAsInt();
  Record.emplace_back(EncodedAttrs);
```
This looks a little bit confusing though, as we don't seem to be _directly_ manipulating MemoryEffects encoding anymore (as it used to be the case).

https://github.com/llvm/llvm-project/pull/120783