[Mlir-commits] [mlir] [MLIR] Introduce support for early exits (PR #166688)

[Tim Noack]

timnoack wrote:

A bigger one: 
`getPropertiesStorageUnsafe()` does not account for the 8 byte offset of num-breaking-control-region (`getPropertiesStorage() `does!), but `Op::getProperties()` uses `getPropertiesStorageUnsafe()` directly:
```c++
  template <typename T = ConcreteType>
  InferredProperties<T> &getProperties() {
    if constexpr (!hasProperties())
      return getEmptyProperties();
    return *getOperation()
                ->getPropertiesStorageUnsafe()
                .template as<InferredProperties<T> *>();
  }
```

In `Operation::getPropertiesStorage()` you worked around this by manually adding the offset:
```diff
   /// Returns the properties storage.
   OpaqueProperties getPropertiesStorage() {
-    if (propertiesStorageSize)
-      return getPropertiesStorageUnsafe();
+    if (propertiesStorageSize) {
+      void *properties =
+          reinterpret_cast<void *>(getTrailingObjects<detail::OpProperties>());
+      if (isBreakingControlFlowFlag)
+        properties =
+            reinterpret_cast<void *>(reinterpret_cast<char *>(properties) + 8);
+      return {properties};
+    }
     return {nullptr};
   }
```

To be honest, I dont quite understand the reason behind this. Wouldnt it make more sense to just apply the 8 byte offset in `getPropertiesStorageUnsafe()` directly (which automatically fixes `Op::getProperties()` and `Operation:: getPropertiesStorage()`, and it keeps `getPropertiesStorage()` consistent with `getPropertiesStorageUnsafe()`)?

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