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?

Update patch from previous comments.

Updated patch based on previous comments. Indeed, changing AArch64TTIImpl::getMemoryOpCost for both load and store was wrong, since <4 x i8> loads are still scalarized. I have changed to just adjust cost for stores.

Updated patch based from previous comment.

Updated patch from previous comments.

In D48332#1136937, @efriedma wrote:

I wonder if we should prefer to widen <2 x i8> and <4 x i8> to <8 x i8> instead of promoting to <4 x i16>. It would make stores like this a bit cheaper. Maybe an interesting experiment at some point (mostly just modifying AArch64TargetLowering::getPreferredVectorAction, I think, and seeing what happens to the generated code).

Indeed the machine was using for speccpu2006 was not best suitable, I used a different one now (tx1, A57) with extra care to lower variance (cpu binding, services disabled, etc) and it indeed showed a better result:

For some reason I did not attach the meant comments in this update. This is an update of the previous patch with an extended analysis. I checked a bootstrap build TargetTransformation::shouldMaximizeVectorBandwidth enabled for both armhf (r332595) and powerpc64le (r332840). On armhf I did not see any regression, however on powerpc64le I found an issue related on how current code handles the MaximizeBandwidth option. The testcase 'Transforms/LoopVectorize/PowerPC/pr30990.ll' explicit sets vectorizer-maximize-bandwidth to 0, however the code checks for:

In fact I couldn't find any code generation different with and without this patch on speccpu2000. Most likely the performance difference is expected runtime variance from speccpu2000 (I do see a lot in some components).

In D46010#1088258, @evandro wrote:

I tired this patch on Exynos and the performance of SPEC CPU2000 was virtually neutral, even if slightly negative overall in the integer score. Perhaps the defaults costs are too optimistic about the relative difference between multiply and add?

But I do agree that there is not much gain in making the division by constant cost logic generic. I changed by moving the logic on aarch64 target cost class.

In D46010#1087835, @rengolin wrote:

In D46010#1078477, @zatrazz wrote:

I should not change other architectures than AArch64 because 'isArithmeticDivFast' is a new method (only used by this patch).

This still looks very AArch64-y to me. Those two ISD nodes are specific to the case you're working on. I'm sure there are a lot more "divs" than that. :)

but since the division by constant is a platform agnostic transformation I though it would make sense to check the same check on a platform agnostic manner.

It is platform agnostic if you cover all cases that a platform agnostic constant divide function should. Right now, it's specific to this one issue in this one arch.

Ping now that https://reviews.llvm.org/D46009 has been committed.

Update patch from previous comments.

In D46009#1083782, @SjoerdMeijer wrote:

Sorry, I needed to get up to speed here, also with this hacker's delight trick.
I think it would be good if you add some comments to LowerMULH what we are trying to do here.

In D46283#1083350, @rengolin wrote:

This seems like an improvement, but we have to be careful with wide variations and little gain. The geomean is almost null but the standard deviation is higher than 75% of the results.

This is not a good sign and is a reflection of what I have complained about exchanging numbers and not tracking which benchmarks get worse/better, then risking hurting more one side than another in the long term.

The patch sets the default to *all* AArch64 cores, which is a wide range and you have only tested on one.

To enable such flag by default, we'd need to run on more cores (preferably including A53) as well as other vendors (like Apple, Samsung, Qualcomm), which this patch is likely to impact.

@t.p.northover @evandro @mcrosier please have a look.

Update patch from previous comments.

In D46010#1077903, @rengolin wrote:

Do we have similar cost tests for x86, Arm and others? It would be nice to make sure the change to BasicTTIImplBase didn't break other targets' costs.

Ping (x4)

Ping (x3).

In D40134#939153, @kubamracek wrote:

Is there a bot that builds for this architecture?

Ping (x2).

Ping.

In D29586#672507, @m.ostapenko wrote:

In D29586#672380, @zatrazz wrote:

In D29586#672157, @m.ostapenko wrote:

So, it seems that LSan's operability depends on what compiler is used to build it :(.

It is a know issue [1] [2] and in fact it a little more complicated, it depends on the flags used as well:

if you mix thumb and arm code fast unwinder won't work. I am not sure which option would be better,

but I am afraid to get a reliable stacktrack on arm (with mixed objects and function built with different compilers)
we will need to use a external unwinder (maybe libunwinder). But it will have the side-effect of making
backtrace way slower.

[1] https://llvm.org/bugs/show_bug.cgi?id=22741
[2] https://llvm.org/bugs/show_bug.cgi?id=18505

Right, but I thought it affects only Thumb (that might be mixed with arm) but it seems that even arm mode is affected depending on GCC version. This complicates enabling tests on arm targets, but still, It would be nice to have LSan available on arm.

In D29586#672157, @m.ostapenko wrote:

In D29586#668769, @m.ostapenko wrote:

In D29586#668050, @rengolin wrote:

You seem to be enabling it on soft and hard float. Did you test on both environments? What type of hardware did you test this?

I've tested the patch on two configurations:

1. Linaro Ubuntu 14.04 (Trusty) on Arndale board. It's a hard float system, right?

Oh, things become more complicated... I see many test failures with host GCC 4.8 because fast unwinder failed to get backtraces for mallocs. This happens because __interceptor_malloc, compiled by GCC 4.8, didn't properly save frame pointer register:

00000000 <__interceptor_malloc>:
   0:   e92d 47f0       stmdb   sp!, {r4, r5, r6, r7, r8, r9, sl, lr}
   4:   f5ad 6d85       sub.w   sp, sp, #1064   ; 0x428
   8:   4d29            ldr     r5, [pc, #164]  ; (b0 <__interceptor_malloc+0xb0>)
   a:   4b2a            ldr     r3, [pc, #168]  ; (b4 <__interceptor_malloc+0xb4>)
   c:   af04            add     r7, sp, #16
   e:   447d            add     r5, pc

But if compile with clang (3.3, 3.5 or trunk) or GCC 4.9, the fp is saved properly:

00000000 <__interceptor_malloc>:
   0:   e92d4ff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, lr}
   4:   e28db01c        add     fp, sp, #28
   8:   e24dd024        sub     sp, sp, #36     ; 0x24
   c:   e24ddb01        sub     sp, sp, #1024   ; 0x400
  10:   e1a08000        mov     r8, r0

So, it seems that LSan's operability depends on what compiler is used to build it :(.

Updated patch based on previous comments.

In D27711#624244, @ruiu wrote:

Looks like Rafael made a comment?

Ping.

Updated patch based on previous comments.

In D27308#612006, @meadori wrote:

LGTM. It is constant with my understanding of: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf

In looking at the surrounding similar relocations, shouldn't R_AARCH64_LDST128_ABS_LO12_NC be 0xFF0?

In D25079#557611, @peter.smith wrote:

• LLD doesn't have a concept of common-page-size and max-page-size so if 64k page size is chosen all output sections are aligned to 64k boundary as well as program segments.

I do not seem any impending issue with this patch, however we must be aware that since max alignment is used as the default one for page alignment for some sections it might interfere with both memory utilization and relro setup. There is some information on a recent powerpc64le issue that occurred from a similar change in binutils [1].

Ping (x2).

Ping.

Ping.

I think patch should be safe now.

In D23420#513824, @jroelofs wrote:

In D23420#513820, @zatrazz wrote:

Yes, although in pratice for shared libraries this is not an issue (at least on Linux with current linker strategies). And I open for suggestion on how to proceed in this case since we have some other options:

1. Add the required soft-sp implementations when building for Linux (this might happen not only on aarch64, but any other ABI that defines long double using fallback soft-fp)

Are the softfp symbols you need not contained in libgcc.a?

Yes, although in pratice for shared libraries this is not an issue (at least on Linux with current linker strategies). And I open for suggestion on how to proceed in this case since we have some other options:

Yes, but my understaning is the proposed link order will force libc++ to link against _Unwind* symbols from libunwind (this is what I am observing also).

What about this version? The only difference is for libunwind libgcc is still included.

I tried to find why exactly libgcc has to appear before and after but I couldn't get any information from commit history. On my system (aarch64/linux) I am also getting "-lc++" "-lm" "-lgcc_s" "-lgcc" "-lpthread" "-lc" "-lgcc_s" "-lgcc".

In D23318#510362, @rengolin wrote:

Reading again, it seems you're just reimplementing a similar logic, with a known global, which is safer than relying on glibc's own copy. So, even for gcc using TSAN, there will be no deviation.

In D23318#510050, @rengolin wrote:

Adding a few people, as I'm on holiday.

I worry that we'll end up with our own private implementation, and create problems for TSAN in the future. Or maybe I didn't understand the problem... :-)

I simplified the patch a bit by removing the allocator64 define usage. I also updated the sync comments.

Ping.