[llvm] Fix: Distinguish CFI Metadata Checks in MergeFunctions Pass (PR #65963)
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 14 08:48:06 PDT 2023
dexonsmith wrote:
> > Can you clarify whether functions should be excluded from merging if the CFI metadata referenced by `llvm.type.test` matches exactly, or only if they are different? (What if they have different identities (`distinct`) but are structurally equivalent?)
>
> If they have different identities but are structurally the same, as in the test I created:
>
> ```
> !10 = !{i64 16, !11}
> !11 = distinct !{}
> !21 = !{i64 16, !22}
> !22 = distinct !{}
> ```
>
> Then CFI will create distinct checks for each of these and they are NOT equal and NOT valid to be merged. I am not the author so I can't explain the exact specifics, but IIUC when a function is internal to the module, CFI will use empty, distinct metadata as placeholders for the final checks.
You only answered the parenthesized question :).
For the first question, I had a look at [the documentation](https://www.llvm.org/docs/TypeMetadata.html). In the examples, there are no `distinct` nodes. Looks like global types that come up like this do not use `distinct`, and are candidates for merging. `distinct` must come up for function-local types. This is likely why merging isn't valid.
(Just to double-check: is `llvm.type.test` being (correctly) rejected for function merging for global types (not `distinct`) when it's not structurally equal? (I.e., pointing at two different global types.) I'm assuming "yes" below.)
----
Perhaps we don't need to compare functions at all; instead, we can filter out functions as candidates for merging if they have (non-debug info) intrinsics that reference `distinct` metadata. You're never comparing metadata. You're just checking it.
I suggest:
- Move the helper function to `MergeFunctions.cpp` and make it a top-level `static` function (local to the file)
- Rename it to `hasDistinctMetadataIntrinsic`
- Use it as a filter to skip over / reject a function
(Or, is there already a place early where functions are rejected as candidates for merging? If so, maybe this logic should be moved there...)
I think you should use logic something like this (I looked at BasicBlock.h to find how to skip debug info):
```
lang=c++
for (const BasicBlock &BB : ...) {
for (const Instruction &I : BB.instructionsWithoutDebug()) {
if (!isa<IntrinsicInst>(I))
continue; // Only intrinsics can reference metadata.
// Iterate through operands, and return true if you find a distinct metadata operand.
}
}
return false;
```
https://github.com/llvm/llvm-project/pull/65963
More information about the llvm-commits
mailing list