[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