@junaire has been looking into this and put up D117480 as an option to extend the intrinsic to have a dedicated order argument.

Hi @fhahn, I wonder if we should continue working on this as I remember one of the reviewers was doubted this change.

In D117480#3248421, @nikic wrote:

I don't get this change. We already use reassoc FMF to allow a non-ordered reduction, what's the purpose of the new flag?

This also needs a LangRef update.

Please note that this patch is WIP.

@junaire did you already get commit access or should I commit this change on your behalf?

In D116736#3230040, @fhahn wrote:

LGTM, thanks!

The last __builtin_reduce_add will be separated into another one.

Are you planning on putting up a patch for this one as well? What makes add a bit different is that ‘llvm.vector.reduce.fadd.*’ can only perform reductions either in the original order or in an unspecified order. For the extension, we need a particular evaluation order (reduction tree adding adjacent element pairs). Technically this order is required for all reduction builtins, but for integers the order doesn't matter, same for min/max.

fix the comment.

After Aarron's comments, I realize what I really want to do is add a new
ASTMatcher that matches declarations what in the top level namespace like std.

In D115429#3219563, @fhahn wrote:

LGTM, thanks!

In D116161#3219343, @fhahn wrote:

In D116161#3219149, @junaire wrote:

In D116161#3219080, @fhahn wrote:

@junaire please let me know if you want me to land this on your behalf.

Yeah, thanks a lot!

You can use:
Jun Zhang
jun@junz.org

Committed! After D115429, I'd recommend you obtain commit access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

In D115429#3219338, @fhahn wrote:

This now needs a rebase after landing 5c57e6aa5777, then it should be good to go.

rebase to main.

In D116161#3219080, @fhahn wrote:

@junaire please let me know if you want me to land this on your behalf.

Refactor code a little bit and fix wrong names.

Extend emitUnaryBuiltin instead of adding a new overload, also apply it to __builtin_elementwise_abs.

In order to use emitUnaryBuiltin in other cases, I changed the function interface.
This allows us to use it in all Builder.CreateUnaryIntrinsic() cases, but will make
the function body very small.

In D116161#3209292, @fhahn wrote:

In D116161#3209286, @junaire wrote:

35:  %0 = load float, float* %f1.addr, align 4 
36:  %1 = load float, float* %f1.addr, align 4 
37:  %elt.abs = call float @llvm.fabs.f32(float %1)

It looks like the argument expression is evaluated twice. Did you remove the Value *Op0 = EmitScalarExpr(E->getArg(0)); calls?

I confirmed that we can use emitUnaryBuiltin in the cases you pointed out. Please see the logs below:

$ path/to/llvm-project/build/bin/clang -cc1 -internal-isystem /path/to/llvm-project/build/lib/clang/14.0.0/include -nostdsysteminc -triple x86_64-apple-darwin /path/to/llvm-project/clang/test/CodeGen/builtins-elementwise-math.c -emit-llvm -disable-llvm-passes -o - | /path/to/llvm-project/build/bin/FileCheck /path/to/llvm-project/clang/test/CodeGen/builtins-elementwise-math.c
/path/to/llvm-project/clang/test/CodeGen/builtins-elementwise-math.c:16:17: error: CHECK-NEXT: expected string not found in input
 // CHECK-NEXT: call float @llvm.fabs.f32(float [[F1]])
                ^
<stdin>:35:43: note: scanning from here
 %0 = load float, float* %f1.addr, align 4
                                          ^
<stdin>:35:43: note: with "F1" equal to "%0"
 %0 = load float, float* %f1.addr, align 4
                                          ^
<stdin>:37:13: note: possible intended match here
 %elt.abs = call float @llvm.fabs.f32(float %1)
            ^

Should also be used here?

Fix wrong usage.

Update the existing place that can use emitUnaryBuiltin.

Update the existing place that can use emitUnaryBuiltin.

In D116161#3206447, @fhahn wrote:

In D116161#3206442, @junaire wrote:

Update the existing place that can use emitUnaryBuiltin.

I meant just update the existing uses *without* adding floor, roundeven, trunc, so this change should be NFC (non-functional change)

Sorry, It seems that the base branch is wrong, reupdate it.

Update the existing place that can use emitUnaryBuiltin.

Sounds good! Might be good to split the emitUnaryBuiltin changes off into a separate change?

• rebase to main.
• rename PrepareBuiltinReduceMathOneArg to PrepareBuiltinReduceMathOneArgCall.

Hi, @aaron.ballman I'm sorry for not updating the patch in time because I'm preparing for my school final exam :-(
One thing I want to mention is that __builtin_elementwise_roundeven is actually been added in the RFC during the code review. You can find it in D111529.

"Prevailing rounding mode" is not super-useful, other than as a spelling for round-to-nearest-ties-to-even (IEEE 754 default rounding). Outside of a FENV_ACCESS ON context, there's not even really a notion of "prevailing rounding mode" to appeal to. I assume the intent is for this to lower to e.g. x86 ROUND* with the dynamic rounding-mode immediate.

I would recommend adding __builtin_elementwise_roundeven(T x) instead, which would statically bind IEEE default rounding (following TS 18661-1 naming) without having to appeal to prevailing rounding mode, and can still lower to ROUND* on x86 outside of FENV_ACCESS ON contexts, which is the norm for vector code (and FRINTN unconditionally on armv8). I think we can punt on rint/nearbyint for now, and add them in the future if there's a need.

Format the patch.

Add a helper function to resue code. I'm not good at naming functions,
so I just oveloaded emitUnaryBuiltin.

Format patch.

Thanks for accepting this patch! I would appreciate it if someone is willing to commit this for me since I don't have commit access ;D

Rename SemaBuiltinReduceMathPreCheck to PrepareBuiltinReduceMathOneArg

Explicitly speak out types for Arg, but don't repeat the vector type name.

The RFC does not mention this builtin. It specifies __builtin_elementwise_rint and __builtin_elementwise_round. Is there a reason why this uses roundeven instead and should there be other rounding options provided at the same time?

In D115231#3194372, @RKSimon wrote:

Any reason not to handle builtin_reduce_xor + builtin_reduce_or as well in the same patch since they are very similar?

Ping.

Ping.

Fix wrong code logic.

Improve diagnostic message.

• Add extra type check to the vector elements.
• Add comment about type restriction.
• polish tests a little bit.

In D114688#3176567, @aaron.ballman wrote:

In D114688#3176433, @fhahn wrote:

I think it might be good to split off the refactoring of SemaBuiltinElementwiseMathOneArg-> PrepareBuiltinElementwiseMathOneArgCall and the update for BI__builtin_elementwise_abs into a separate, non functional change (NFC).

FWIW, I would not be opposed to that, but I don't insist either. LGTM!

Fix typos

No more lambdas

I didn't know if I understand Aaron's words right, but I think I can update the patch first...

Apply some awesome suggestions.

Adjust code style

Reuse existing code.