[PATCH] D38596: Implement attribute target multiversioning

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
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",
----------------
erichkeane wrote:
> 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.
OK, that sounds reasonable. Please rephrase this comment; the current wording could be read as suggesting this is in some way derived from the GCC source code, which is clearly not what you did, nor what we're suggesting that people updating this list do.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:421-423
+      Diags.Report(
+          cast<FunctionDecl>(GD.getDecl())->getFirstDecl()->getLocation(),
+          diag::err_target_without_default);
----------------
erichkeane wrote:
> 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.
I think we should reject any case where new versions of the function are added after the first use. Generally we want our extensions to support eager code generation, even in cases where that means we reject certain setups that GCC copes with.


https://reviews.llvm.org/D38596