[Mlir-commits] [clang] [llvm] [mlir] Extend MemoryEffects to Support Target-Specific Memory Locations (PR #148650)

[Nikita Popov]

nikic wrote:

At a high level, this looks fine, but I have some concerns on the details. In particular, the implementation currently uses the term "inaccessible memory" to refer to both the inaccessiblemem location (e.g. in `MemoryEffects::inaccessibleMemOnly()`) and to all inaccessible memory, including target memory (e.g. in `MemoryEffects::onlyAccessesInaccessibleMem()`). I think this is something we need to be careful about, as it's easy to make a mess.

What I would suggest is the following hierarchy of locations: `inaccessiblemem` contains `other_inaccessiblemem` and `target_mem`. `target_mem` contains `target_mem0`, etc. We'll likely split out more specific locations about of `inaccessiblemem` in the future (like for example fp control register for constrained intrinsics and similar), and I think this will simplify it. Code needs to explicitly chose whether it is talking about all inaccessible memory (usually the case for accessors) or a specific subset (usually for constructors). This should also help with IR printing, as you won't have to spell out all target locations in common cases.

Another thing we need to consider when adding new locations is bitcode upgrade. If there is old bitcode with inaccessiblemem: readwrite, we probably need to propagate that to the new target locations.

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