[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. Relanded: fc53eb69c26cdd7efa6b629c187d04326f0448ca Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/new/ https://reviews.llvm.org/D51650

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. In D51650#3127097 , @aaron.ballman wrote: > In D51650#3126569 , @akuegel wrote: > >> Since it is not clear whether the semantic change was intended, I think it >> makes sense to revert

[PATCH] D51650: Implement target_clones multiversioning

danielkiss added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3290-3296 + // Ensure we don't combine these with themselves, since that causes some + // confusing behavior. + if (const auto *Other = D->getAttr()) { +S.Diag(AL.getLoc(), diag::err_disallowed_d

[PATCH] D51650: Implement target_clones multiversioning

aaron.ballman added a comment. In D51650#3126569 , @akuegel wrote: > Since it is not clear whether the semantic change was intended, I think it > makes sense to revert the patch for now. If it is intended, it might be good > to mention it in the change d

[PATCH] D51650: Implement target_clones multiversioning

akuegel added a comment. Since it is not clear whether the semantic change was intended, I think it makes sense to revert the patch for now. If it is intended, it might be good to mention it in the change description, so that people are warned. Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D51650: Implement target_clones multiversioning

jgorbe added a comment. This breaks some existing code (we have found this issue building the JPEG XL library at github.com/libjxl). This is a very simplified version of the problem: #pragma clang attribute push(__attribute__((target("sse2,ssse3"))), apply_to = function) __attribute__((targ

[PATCH] D51650: Implement target_clones multiversioning

This revision was automatically updated to reflect the committed changes. Closed by commit rG9deab60ae710: Implement target_clones multiversioning (authored by erichkeane). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D51650?vs=386573&id=386597#toc Repositor

[PATCH] D51650: Implement target_clones multiversioning

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a nit with naming/wording. Feel free to land without additional review (unless you want more review, of course!). Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9868-9869 InGr

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 386573. erichkeane added a comment. added C++ tests this time, changed how dupes are diagnosed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/new/ https://reviews.llvm.org/D51650 Files: clang/include/clang/AST/Decl.h clang/include/clan

[PATCH] D51650: Implement target_clones multiversioning

aaron.ballman added a comment. In D51650#3124859 , @erichkeane wrote: > Did all the things Aaron asked for, but required adding 'lambda not supported > yet' logic for this. I don't see the .cpp test file, did it get dropped by accident? Also, pre-commit

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 386542. erichkeane marked 5 inline comments as done. erichkeane added a comment. Did all the things Aaron asked for, but required adding 'lambda not supported yet' logic for this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/new/ https://

[PATCH] D51650: Implement target_clones multiversioning

aaron.ballman added a comment. Overall, I think this LGTM, but I did find a few nits. Can you fix the clang-format issues? Also, I'd like to see some C++ test coverage that shows how this works on template (partial) specializations, lambdas (with GNU-style syntax), and overloaded functions. If

[PATCH] D51650: Implement target_clones multiversioning

danielkiss accepted this revision. danielkiss added a comment. This revision is now accepted and ready to land. In D51650#3121789 , @erichkeane wrote: > But nit made. NIT: clang-format issues still present. Maybe you need to update your local clang-forma

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 386205. erichkeane marked an inline comment as done. erichkeane added a comment. For the rest of multiversioning we count on the optimizer to remove variants made irrelevant, but I'm not sure opt can do anything like that yet :) But nit made. CHANGES S

[PATCH] D51650: Implement target_clones multiversioning

danielkiss added a comment. Thanks for the rebase! Looks got to me, `-march` may make the default same as one of the clone, in this case maybe we don't need to create two versions of the function. This could be solved later IMHO. Comment at: clang/lib/Sema/SemaDecl.cpp:1066

Re: [PATCH] D51650: Implement target_clones multiversioning

I think you are these days too :) My offer was "past-past-me". I think you're probably ok here, I did a rough scan, but getting someone like Aaron for the attribute support would be good. On Fri, Nov 5, 2021 at 12:00 PM Erich Keane via Phabricator < revi...@reviews.llvm.org> wrote: > erichkeane a

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. Sadly, I think _I_ am the multiversioning expert (or at least, past-me was), so I'm hoping some of the reviewers @danielkiss can get to join will be able to read/understand this stuff for a quality review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/

[PATCH] D51650: Implement target_clones multiversioning

echristo added a comment. FWIW I'm a bit rusty in this area myself, but thanks for doing this. Let's see if we can't get Aaron to continue reviewing :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/new/ https://reviews.llvm.org/D51650

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 385089. erichkeane added a comment. Another rebase, as requested. I am not particularly familiar with this code anymore, so my responses to reviews might not be particularly well informed, but I'm hoping that rust shakes off through the review :) CHANG

[PATCH] D51650: Implement target_clones multiversioning

danielkiss added a comment. In D51650#3096487 , @erichkeane wrote: > This needs rebasing/probably some cleanup, plus code review. If you have > active/proficient CFE code reviewers who could take a look, I'd be willing to > spend some time rebasing. Th

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. In D51650#3095997 , @danielkiss wrote: > Hi @erichkeane, > > At Arm we are going to add the multiversioning support for Arm targets[1][2]. > It would be nice to land this change because we could build top of it. > Please let me

[PATCH] D51650: Implement target_clones multiversioning

danielkiss added a comment. Hi @erichkeane, At Arm we are going to add the multiversioning support for Arm targets[1]. It would be nice to land this change because we could build top of it. Please let me know how can I help. [1]. https://github.com/ARM-software/acle/pull/21 CHANGES SINCE LAST

[PATCH] D51650: Implement target_clones multiversioning

lebedev.ri added a comment. In D51650#2454830 , @erichkeane wrote: > Since there seems to be at least a little renewed interest in this from a > handful of people lately, I spend the time to rebase this and do some minor > cleanup tasks. FWIW yes, i'm b

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 311890. erichkeane added a comment. Since there seems to be at least a little renewed interest in this from a handful of people lately, I spend the time to rebase this and do some minor cleanup tasks. CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. In D51650#2418749 , @lebedev.ri wrote: > In D51650#1305509 , @erichkeane > wrote: > >> Fix @rsmith s comments, rebase on the big CPUDispatch refactor. > > Ping. What's the status here?

[PATCH] D51650: Implement target_clones multiversioning

lebedev.ri added a comment. Herald added a subscriber: jdoerfert. In D51650#1305509 , @erichkeane wrote: > Fix @rsmith s comments, rebase on the big CPUDispatch refactor. Ping. What's the status here? CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 174950. erichkeane added a subscriber: grooverdan. erichkeane added a comment. Fix @rsmith s comments, rebase on the big CPUDispatch refactor. https://reviews.llvm.org/D51650 Files: include/clang/AST/Decl.h include/clang/Basic/Attr.td include/clang

[PATCH] D51650: Implement target_clones multiversioning

rsmith added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:961-970 if (const auto *FD = dyn_cast(ND)) if (FD->isMultiVersion() && !OmitMultiVersionMangling) { if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion()) AppendCPUSpe

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. Thanks for the feedback @rsmith! I'm working through the lambda issue and a few other things, but will get to this as soon as I can. Comment at: include/clang/Basic/Attr.td:2031-2042 +mutable unsigned ActiveArgIndex = 0; +void AdvanceActive

[PATCH] D51650: Implement target_clones multiversioning

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:2031-2042 +mutable unsigned ActiveArgIndex = 0; +void AdvanceActiveArgIndex() const { + ++ActiveArgIndex; + while(ActiveArgIndex < featuresStrs_size()) { +if (std::find(featuresStrs_be

[PATCH] D51650: Implement target_clones multiversioning

erichkeane added a comment. Based on feedback from @rsmith I've been trying to get this to work with Lambdas like GCC does, though I'm quite confused about how to go about it here. It seems that the lambda definition call operator is deferred, however I'm having a hard time making sure that th

[PATCH] D51650: Implement target_clones multiversioning

erichkeane updated this revision to Diff 164251. erichkeane marked 9 inline comments as done. erichkeane added a comment. fix aaron's comments. https://reviews.llvm.org/D51650 Files: include/clang/AST/Decl.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic

[PATCH] D51650: Implement target_clones multiversioning

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Thank you for this! I have some cursory review comments, and possibly more later. Comment at: include/clang/Basic/AttrDocs.td:1600 + let Content = [{ +Clang supports the GNU style ``__attribute__((target_c

[PATCH] D51650: Implement target_clones multiversioning

erichkeane created this revision. erichkeane added reviewers: thiagomacieira, aaron.ballman. As discussed here: https://lwn.net/Articles/691932/ GCC6.0 adds target_clones multiversioning. This functionality is an odd cross between the cpu_dispatch and 'target' MV, but is compatible with neither.