[llvm] [MemProf] Add option to match summary and definition strictly (PR #83665)

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 8 14:32:26 PST 2024


teresajohnson wrote:

> > Thanks for working on this. Since I believe the test case will assert without the added options, I'm thinking it is probably best to simply always do this in a debug compiler. Can we force enable enable-import-metadata when cloning is enabled in the thin link (or pass down a flag to ComputeCrossModuleImport to enable it)? The checking that fails without this patch should probably also go under an `#ifndef NDEBUG` (looks like I missed adding that).
> 
> Sorry, I don't understand fully.
> 
> 1. I have updated code to match definition and summary default. But I can't understand why we need to distinguish debug compiler and release compiler.

Yeah, re-reading my comment and the code this was wrong - I was thinking about the sanity checking we do to ensure the IR and summary match, but we obviously need the correct summary for more than that. Please ignore that part. Doing this always as you have changed the code though does make sense, since there is a correctness issue if we don't find the right summary.

> 2. I don't understand where to add `#ifndef NDEBUG`.

I was looking at the code that does the sanity checking to compare IR and summary, and that whole block can go under the ifndef, but I can do this separately, it is unrelated to your change now.


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


More information about the llvm-commits mailing list