[Mlir-commits] [mlir] [mlir][DataLayout] Add a default memory space entry to the data layout. (PR #127416)

[Rolf Morel]

================
@@ -318,7 +318,8 @@ combineOneSpec(DataLayoutSpecInterface spec,
            "unexpected data layout entry for built-in type");
 
     auto interface = cast<DataLayoutTypeInterface>(typeSample);
-    if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second))
+    if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second,
----------------
rolfmorel wrote:

Indeed, I noted you using `entriesForID` and was hoping that would sufficient complication of the interface.

> newSpec contains the correct instance of the method

That works for your usage: the `get...Identifier` methods are okay to access this way but `newSpec` itself is just the latest individual spec to get merged into the lists and `areCompatible` will be called with each of these `spec`s in turn (in addition to the `entriesForID` list which is the correct merging of all hitherto specs). Also, one of these `spec`s will define the `newLayout`. Personally, I am getting a waft of code smells.

Having said that, I don't want to hold this up any longer. If could just add a TODO/FIXME regarding this requiring clean-up/reconsideration, that would be much appreciated. Thanks!

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