erichkeane added a comment. Hi Richard, thanks for the review! I'll make another attempt with this based on your feedback, though if you could clarify your suggestion, it would be greatly appreciated.
In https://reviews.llvm.org/D38596#909957, @rsmith wrote: > I'm not entirely happy with the AST representation you're using here. > Allowing multiple declarations of the same entity to have (semantically > distinct) bodies breaks our AST invariants, and will cause things like our > PCH / modules support to fail. This can probably be made to work, but I'd > like to see what that looks like before we commit to modeling a > multiversioned function this way rather than, say, with different > redeclaration chains for each version. I'd experimented with (but not succeeded with) simply preventing merging of the decls, but had a tough time keeping like-versions together for the purposes of the iFunc. Additionally, clarifying the 'call' later on became quite difficult. I can definitely give that another try though. > You should add tests for: > > - the PCH / Modules case (particularly regarding what happens when merging > multiple copies of the same set of functions, for instance when they're > defined inline); I would expect this doesn't currently work with this patch, > because we only ever load one function body per redeclaration chain I'm not sure I know what one of those looks like... is there a good test that I can use to learn the modules implementation? > - multiversioned functions declared inline I don't see anything changing for this situation, but I imagine you have an issue in mind that I'm missing? > - constant expression evaluation of calls to such functions (which should -- > presumably -- either always use the default version or be rejected as > non-constant) I'd say they should always do the default version, but this isn't something I did any work on. I'll look at that. > I would also expect that asking a multiversioned `FunctionDecl` for its > definition would only ever give you the `target("default")` definition or the > one-and-only definition, rather than an arbitrary declaration that happens to > have a body. Right, this is a big catch that I didn't think about... ================ Comment at: lib/Basic/Targets/X86.cpp:1329 + // implementation in in GCC 7.x in gcc/config/i386/i386.c + // ::get_builtin_code_for_version. This list is simplified from that source. + const auto TargetArray = {"avx512vpopcntdq", ---------------- rsmith wrote: > Where did this table and code actually come from? I took the list of things recognized by GCC, ran them through godbolt to see the list. ================ Comment at: lib/Basic/Targets/X86.cpp:1392 + "intel", + "amd"}; + ---------------- rsmith wrote: > Can you replace this list (and the repetition of it elsewhere in this file) > with a `.def` file carrying the same information? I've been working with Craig trying to do something like this, though not with a .def file. So far I've yet to come up with something signifiantly better, but I'm still working on it. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:421-423 + Diags.Report( + cast<FunctionDecl>(GD.getDecl())->getFirstDecl()->getLocation(), + diag::err_target_without_default); ---------------- rsmith wrote: > I'm not happy with this diagnostic being produced by IR generation. We should > diagnose this at the point of first use of the multiversioned function, > within `Sema`. Unfortunately, there is no real way to tell there. A usage could happen before the 2nd definition, so it wouldn't know that it IS a multiversioning at that point. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:2134 + return MangledName; + return (MangledName + "." + Append).str(); +} ---------------- rsmith wrote: > CodeGen should not be inventing mangled names with external linkage. Please > move this to the name mangler. Is this mangling scheme compatible with GCC? > How will we avoid name collisions between this and other uses of dot suffixes? I can definitely extract this over to the name manglers. This mangling scheme is exactly what GCC and ICC do currently. I have no idea how it avoids collisions besides not being able to define the root name with another mechanism. https://reviews.llvm.org/D38596 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
