[PATCH] D117829: [Clang] Add integer add/mul reduction builtins

Fri Jan 21 02:09:03 PST 2022

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