Add support for self referential metadata #895
Conversation
|
@aseyboldt thank you for submitting this PR. I have labelled it as Ready for Review for now. It will hopefully be assigned a reviewer and milestone during the next issue triage meeting on Tuesdays. |
|
@esc Just checking in if a reviewer has been assigned? |
There was a problem hiding this comment.
The situation with self-referential metadata is a bit unclear in LLVM. There are discussion about removing them https://lists.llvm.org/pipermail/llvm-dev/2021-February/148674.html. I'm not sure if the distinct keyword has make this self-referential trick outdated. For now, LLVM still contains code to create self-referential MD. It is done so by first building a metadata node without self-reference then replace the first operand with the self-reference; e.g. https://github.com/llvm/llvm-project/blob/080a07199e8c146a58ef3506cfcbf554eced21fc/llvm/lib/Analysis/LoopInfo.cpp#L1159-L1161.
The patch here adds a self_ref keyword argument to prepend a self-reference to the operands. I think this logic is okay given any self-referential MD node seems to always be just a bag of stuff. I don't think we should overengineer it too much.
Given the code changes here are small, I am ok to accept this patch once the following suggestions are addressed.
llvmlite/ir/module.py
Outdated
| A self referential metadata entry has itself as the first | ||
| operand to ensure uniquencess. These references are never | ||
| cached. |
There was a problem hiding this comment.
Can you add few words to relate this to the self_ref argument? e.g. the self_ref creates a self referential metadata by prepending itself to the operands?.
llvmlite/ir/module.py
Outdated
| """ | ||
| Add an unnamed metadata to the module with the given *operands* | ||
| (a sequence of values) or return a previous equivalent metadata. | ||
| A MDValue instance is returned, it can then be associated to | ||
| e.g. an instruction. | ||
|
|
||
| A self referential metadata entry has itself as the first | ||
| operand to ensure uniquencess. These references are never |
There was a problem hiding this comment.
| operand to ensure uniquencess. These references are never | |
| operand to ensure uniqueness. These references are never |
llvmlite/tests/test_ir.py
Outdated
| def test_self_relf_metadata(self): | ||
| mod = self.module() | ||
| value = mod.add_metadata((), self_ref=True) | ||
| self.assert_ir_line('!0 = !{ !0 }', mod) | ||
| value2 = mod.add_metadata((value,)) | ||
| assert value is not value2 | ||
| self.assert_ir_line('!1 = !{ !0 }', mod) | ||
| value3 = mod.add_metadata((), self_ref=True) | ||
| self.assert_ir_line('!2 = !{ !2 }', mod) | ||
| assert value is not value3 |
There was a problem hiding this comment.
Can you add test for metadata that has other operands?
|
@sklam Thanks, I added a test and extended the docs a little as you suggested. |
Metadata definitions that refer to themselves like
!0 = { !0 }are allowed by llvm, and used to make some metadata entries globally unique (see for instance https://llvm.org/docs/LangRef.html#noalias-and-alias-scope-metadata). It seems this isn't supported by llvmlite currently.In this PR I add support for this by adding a new
self_refargument toModel.add_metadata, that will always return a new metadata entry, but prepending self as the first operand. So this only supports references to self as the first entry, I haven't seen any usage in llvm at other locations however.Update I probably should also mention that I'm still pretty new to llvm, and this proposed API might not actually be the best as a result...