In D67733#1674557, @saugustine wrote:

Thanks so much for fixing this. Really appreciate it.

Ping.

Is this patch ok with windows support now?

Updated patch based on previous comments. Add a pattern to handle i32 return and a testcase for Windows.

Ping now that both D62017 and D62019 have been approved.

Updated patch based on D62026.

Updated patch based on D62026.

In D62017#1505300, @craig.topper wrote:

Can we do this with 2 intrinsics with overloaded result types as I've done for lround/llround in D62026?

LGTM, thanks for the follow-up patch.

So should I handle f16 in a different patch or should I also adapt it on this one?

Ping again now that D61390 has been approved.

Ping.

Updated patch based on the previous comment. The main change is the IBM long double tests which also required both SoftenFloatOp_LROUND and SoftenFloatOp_LLROUND implementation to handle the type correctly.

In D61390#1495541, @craig.topper wrote:

Should we have a PowerPC test for ppcf128?

Updated patch based on previous review.

Updated patch based on previous comments. The changes are:

In D61390#1492891, @craig.topper wrote:

Can you test lround.i32 with -mtriple=i686-unknown with and without -mattr=sse2.

Use utils/update_llc_test_checks.py to generate the checks for at least X86. Try it on the other targets too if it works for them.

Updated patch based on the previous comment. The main change are:

Updated patch based on D61390 changes. The main change is it allows
index both lround/llround by returned type.

Updated patch based on the previous comment. The main change adapt the testcases for D61390
update and handle lroundl/llroundl correctly.

In fact, it turned out that indexing the new lround/llround based on
input argument didn't really make it correctly handled as an expanded
operation. I will dig into exactly why backend is not selecting correctly
based on input argument, so I change it back to previous indexing by
return type.

Updated patch based on previous comments. The changes from the previous version are:

Updated patch based on previous comments.

Ping, only this refactor is missing review for my aarch64 fp materialization optimization.

Ping.

Ping.

Fxied CamelCase (sorry missing the comment).

I added a testcase based on test/CodeGen/ARM/immcost.ll.

Updated patch based on previous comments.

Updated patch based on previous comments. It depends on https://reviews.llvm.org/D58915. I am trying to come up with a testcase to actually test it. The the main difference for the cost analysis is just for some immediate that contains some 16-bit zero or one chunk (which will have the size adjusted from 3 to 2). Since they are not TCC_Free, TCC_Basic, or TCC_Expensive the change does not actually interfere in the further cost analysis.

Updated patch based on previous comments. It depends on https://reviews.llvm.org/D58915 and https://reviews.llvm.org/D58690

Updated patch based on previous comments.

In D58460#1411040, @efriedma wrote:

Not sure I like the duplicated logic in getExpandImmCost; it doesn't have good test coverage, and it could fall out of sync in the future. That said, I've been considering refactoring the code in expandMOVImm anyway, to split the actual instruction emission away from the logic that figures out the appropriate sequence. Basically, the idea would be that instead of returning a number from getExpandImmCost, you return an abstraction of the instruction sequence: an array that contains, for each instruction, the appropriate opcode and immediate. isFPImmLegal just uses the number of elements in the array, while expandMOVImm actually emits instructions based on the array. I think this would shrink the code overall because the logic for building instructions is currently duplicated multiple times. (I was considering it more in the context of adding more possible sequences, but it works here as well.)

New revision based on previous comments. I refactored the logic used on isFPImmLegal to evaluate whether to materialize the FP constant or not by adding a new function on common aarch64 code, AArch64_AM::getExpandImmCost. To avoid code replication I refactored the code by moving some definitions from AArch64ExpandPseudoInsts.cpp.

Updated patch based on previous comment:

Ping.

Patch updated based on previous comments. The isAnyMOVWMovAlias
can catch more cases where we can materialize the floating-point constant
than isLogicalImmediate (128 .0 for instance). The positive zero still need to
be handled as specific case because the resulting fmov will use the zero
register instead of an immediate. The isAnyMOVWMovAlias path is not use
for fp16 (I will need to check further if is safe for all cases).

Updated patch from previous comments. I added an extra check using AArch64_AM::isLogicalImmediate for constants that can be materialized as the immediate operand of a logical instruction. We still need to handle the cases for some 8-bit immediates on fmov using getFPXXImm and positive 0.0.

In D57044#1367943, @efriedma wrote:

Is there some reason we're checking for specific floating-point constants here, as opposed to just calling AArch64_AM::isLogicalImmediate or AArch64_AM::isAnyMOVWMovAlias?

Updated patch with additional tests. I added tests for __builtin_isinf expanded builtin for float, double, and long double (the latter still requires loading the constant).

Ping.

In D55427#1331277, @zatrazz wrote:

In D55427#1331048, @rengolin wrote:

Getting a bit late in this discussion, as we had an internal one just recently.

The change to remove always_inline in a number of libc++ template functions is a good one, especially when the inliner can guess and does a good job already.

In this case, however, because the type is a reference, the inliner would require a lot more effort to inspect the uses (and side-effects).

Improving the inliner here would be a huge hammer, probably increasing compile time for all codes for the minimal benefit of this very special case.

Then perhaps, it would be beneficial and pragmatic, to revert that removal in this special case.

The issue I have to define it per symbol is the hackery it would require to handle _LIBCPP_INTERNAL_LINKAGE and its implications,
or at least add *another* macro to inline some symbols depending of the configuration/etc.

In D55427#1331048, @rengolin wrote:

Getting a bit late in this discussion, as we had an internal one just recently.

The change to remove always_inline in a number of libc++ template functions is a good one, especially when the inliner can guess and does a good job already.

In this case, however, because the type is a reference, the inliner would require a lot more effort to inspect the uses (and side-effects).

Improving the inliner here would be a huge hammer, probably increasing compile time for all codes for the minimal benefit of this very special case.

Then perhaps, it would be beneficial and pragmatic, to revert that removal in this special case.

In D55427#1329797, @mclow.lists wrote:

In D55427#1325285, @zatrazz wrote:

This patch aims to help clang with better information so it can inline
__bit_reference count function usage for both std::biset. Current clang
inliner can not infer that the passed typed will be used only to select
the optimized variant, it evaluates the type argument and type check as
a load plus compare (although later optimization phases correctly
optimized this out).

I'm unclear on the magnitude of the improvement here.
Are we talking a single load + compare instruction in the call to std::count ?
Or something inside the loop?

[ I'm pretty sure that the patch is correct now - but I don't understand how important it is ]

Ping.

This patch aims to help clang with better information so it can inline
__bit_reference count function usage for both std::biset. Current clang
inliner can not infer that the passed typed will be used only to select
the optimized variant, it evaluates the type argument and type check as
a load plus compare (although later optimization phases correctly
optimized this out).

In D55427#1323715, @mclow.lists wrote:

In D55427#1323656, @mclow.lists wrote:

This looks like a behavior change to me.
The old code calls __count_bool_true if the _Tp can be static_cast to bool, and __count_bool_false otherwise.
The new code calls __count_bool_true if the _Tp is exactly bool, and __count_bool_false otherwise.

Sorry; the old code calls __count_bool_true if the static_cast<bool>(value) is true, and __count_bool_false otherwise.

Ping?