erichkeane added a comment. In D112349#3112315 <https://reviews.llvm.org/D112349#3112315>, @v.g.vassilev wrote:
> In D112349#3111927 <https://reviews.llvm.org/D112349#3111927>, @erichkeane > wrote: > >> In D112349#3111873 <https://reviews.llvm.org/D112349#3111873>, @ibookstein >> wrote: >> >>> And how is Cling expecting CFE to deal with partial knowledge situations at >>> the implementation level? To deal with exactly the non-local cases that the >>> current violations address? >>> If there's no prescriptive way forward to dealing with these cases (so >>> they're tech debt without a remediation plan), then as far as I'm concerned >>> this case sits exactly under the same tech-debt umbrella of the existing >>> violations and the way forward is using the same violating solution for >>> this case too. >>> We definitely shouldn't block the IR verification indefinitely on this >>> impasse. Once an acceptable solution is found, this will also be part of >>> that refactor. >> >> My understanding is that the REPL setup is that the 'IR' just needs to be in >> a state where it doesn't require reverts/rewrites later, so that we can do >> partial-back-end-code-generation as we go along. That said, I'm not >> positive as to the implications. I'm just repeating the discussion the CFE >> code-owner made at the time. >> >> IMO, the 'acceptable' solution is to have a way to forward-declare an ifunc >> in IR so that we can fill in the resolver later on. From your description >> earlier, it seems that this would work as we could emit it as an 'unknown >> symbol' (as if it was an undefined function declaration), and would be >> completely implementable in the CFE. >> >> So it would change your plan from earlier to: >> >> When processing cpu_specific, emit the ifunc "x.ifunc", with no resolver; >> When processing cpu_dispatch: >> >> Get/Create the ifunc, then pull up the resolver. >> If the resolver is null (as it should be), create one and update the >> 'ifunc'. >> Generate said resolver. >> > > Speaking of the incremental compilation case, we can experiment with the > clang-repl binary. I am not sure about the details of this discussion but > here is an example that works today: > > llvm/build/bin/clang-repl > clang-repl> __attribute__((cpu_specific(ivybridge))) void > single_version(void){} > clang-repl> void useage() {single_version();} > clang-repl> quit > > What would it be a good example to check if the incremental compilation case > is covered? FWIW, the comments above are about not making it 'worse' at least/adding more wo In D112349#3113054 <https://reviews.llvm.org/D112349#3113054>, @ibookstein wrote: > I feel like we're getting lost in the weeds here. > > At the time a bitcode module is finalized, it is supposed to be in a valid > state. > The LLVM bitcode verifier does not consider GlobalAliases which have either a > null or an undefined aliasee to be valid. The same should have held for > GlobalIFuncs and their resolvers since their inception, and the fact that it > were not so is an oversight. > > There are two separate issues here which we need to decouple: > > - IFuncs with undefined resolvers were never valid, the verification just > happened to be missing. They also misbehave, as demonstrated by my example > above where a TU contains both cpu_specific and a usage, but no cpu_dispatch. > Therefore, we'd not be making anything worse by plugging the current hole in > the verification of GlobalIFunc and keeping cpu_specific/cpu_dispatch > working, using the same method we use to handle aliases or ifuncs that come > after declarations, i.e. cpu_specific emits plain function declarations, > cpu_dispatch upgrades them via takeName+RAUW+eraseFromParent if they already > exist. We'll have made the verification more robust, fixed a bug when there's > a TU where there's cpu_specific + usage but no cpu_dispatch, and not have > incurred more tech debt by doing so (since that tech debt already exists, and > a solution would need to address all these cases in exactly the same way). > - Clang-repl/CGM needs infrastructure for dealing with this sort of > 'backtracking' in incremental compilation use-cases (declaration gets > upgraded to alias, declaration gets upgraded to ifunc). If it needs IR > changes for that, then they should be designed, agreed upon, and integrated. > We're not going to have a decision made in this closed PR discussion that > either GlobalAliases or GlobalIFuncs support a declaration state all of a > sudden. Such decisions could have cross-cutting ramifications in that there > would all of a sudden be 3 ways to represent things that are equivalent > to/get lowered to function declarations, rather than one. Limiting ourselves > to not using the existing solution for these use-cases/bugs in normal > compilation with the hope that it will ease the creation of a solution for > incremental compilation of the same use-cases is self-defeating. > > As for clang-repl, I've so far tried the following; I get the sense that I'm > poking around in less well-specified places (asserts and null-deref crashes): > > itay ~/llvm-project/build (main)> bin/clang-repl > clang-repl> #include <stdio.h> > clang-repl> __attribute__((cpu_dispatch(generic))) void a(void); > clang-repl> __attribute__((cpu_specific(generic))) void a(void) { > puts("hi"); } > In file included from <<< inputs >>>:1: > input_line_2:1:55: warning: body of cpu_dispatch function will be ignored > [-Wfunction-multiversion] > __attribute__((cpu_specific(generic))) void a(void) { puts("hi"); } > ^ > clang-repl> auto b = (a(), 5); > clang-repl: > /home/itay/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1683: void > llvm::AsmPrinter::emitGlobalIFunc(llvm::Module &, const llvm::GlobalIFunc &): > Assertion `GI.hasLocalLinkage() && "Invalid ifunc linkage"' failed. > fish: 'bin/clang-repl' terminated by signal SIGABRT (Abort) > itay ~/llvm-project/build (main) [SIGABRT]> bin/clang-repl > clang-repl> extern int g_a __attribute__((alias("g_b"))); > In file included from <<< inputs >>>:1: > input_line_0:1:31: error: alias must point to a defined variable or function > extern int g_a __attribute__((alias("g_b"))); > ^ > fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error) > itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl > clang-repl> void foo(void) __attribute__((ifunc("foo_resolver"))); > In file included from <<< inputs >>>:1: > input_line_1:1:31: error: ifunc must point to a defined function > void foo(void) __attribute__((ifunc("foo_resolver"))); > ^ > fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error) > itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl > clang-repl> void foo(void); > clang-repl> void bar(void) __attribute__((alias("foo"))); > In file included from <<< inputs >>>:1: > input_line_1:1:31: error: alias must point to a defined variable or function > void bar(void) __attribute__((alias("foo"))); > ^ > fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error) > itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl > clang-repl> void foo(void); void bar(void) __attribute__((alias("foo"))); > void foo(void) {} > In file included from <<< inputs >>>:1: > input_line_0:1:47: error: alias must point to a defined variable or function > void foo(void); void bar(void) __attribute__((alias("foo"))); void > foo(void) {} > ^ > fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error) > itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl > clang-repl> void foo(void) {} > clang-repl> void bar(void) __attribute__((alias("foo"))); > In file included from <<< inputs >>>:1: > input_line_1:1:31: error: alias must point to a defined variable or function > void bar(void) __attribute__((alias("foo"))); > ^ > fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error) To me the 'not in the weeds' part is, "How do I get CPU-dispatch/CPU-specific to work without RAUW, since that is offensive to the CFE code owner? Additionally, I found that some of the examples without the defined resolver actually DO work in my downstream, though I don't know what changes we make to make that happen. So adding this limitation in actually breaks my downstream. > The LLVM bitcode verifier does not consider GlobalAliases which have either a > null or an undefined aliasee to be valid. The same should have held for > GlobalIFuncs and their resolvers since their inception, and the fact that it > were not so is an oversight. Citation Needed here. Is there a design document that specifies this, or is this your opinion? If its the latter, the implementation was the documentation, so this is still a breaking change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
