fhahn added a subscriber: junaire. fhahn added a comment. Thanks for the patch!
> For other reductions, we've tried to share builtins for float/integer > vectors, but the fadd/fmul reduction builtins also take a starting value > argument. Technically I could support float by using default values, but > we're probably better off with specific fadd/fmul reduction builtins for both > arguments. Just to double check, you mean LLVM intrinsics here, right? As specified, the Clang `__builtin_reduce_add` should support both integer and floating point reductions. Do you think that's problematic? If so, we should update the spec. For float reductions, the key questions is how to lower it. Unfortunately `llvm.vector.reduce.fadd` reductions at the moment are either unordered or sequential, but we need a particular order. @junaire has been looking into this and put up D117480 <https://reviews.llvm.org/D117480> as an option to extend the intrinsic to have a dedicated order argument. That would make it easy for backends like AArch64 to select the right reduction instruction. Alternatively clang could also create the expanded reduction tree to start with. But we really want to lower this to native reduction instructions if we can. In D117829#3259588 <https://reviews.llvm.org/D117829#3259588>, @RKSimon wrote: > I should mention - according to > https://clang.llvm.org/docs/LanguageExtensions.html `__builtin_reduce_add()` > already exists, which I don't think is true. Do you mean it is listed in the docs but not implemented yet? That's true, the docs specified the whole set of proposed builtins from the start, regardless of implementation status. FWIW, I think we intentionally did not specify `__builtin_reduce_mul` to start with, because it is very prone to overflows for integer vectors. Not saying that we cannot add it, but it would at least require updating the extension documentation. @scanon might have additional feedback. In any case, it might be good to only handle the `add` case in this patch. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:2263 // These builtins support vectors of integers only. + case Builtin::BI__builtin_reduce_add: + case Builtin::BI__builtin_reduce_mul: ---------------- `_add` should also support floats, add a TODO? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117829/new/ https://reviews.llvm.org/D117829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
