In D53927#2786879, @georgemitenkov wrote:

Thanks @rengolin! I guess in this case we have to see what @steleman and @joelkevinjones think and if they are interested (As well as other reviewers).

It would be really great to have this functionality. With the recent patch (https://reviews.llvm.org/D95373) math intrinsics can be changed into calls for SIMD libraries, and having Sleef there just completes the picture. I am wondering if may be it is better to discuss this on the mailing list? (Sometimes reverted patches are lost for reviewers)

In D53927#1411714, @rengolin wrote:

One minor update:

In D53927#1400128, @rengolin wrote:

@bryanpkc debug build makes sense, thanks! @steleman, I'd rather just order now and try to rector later. This patch has changed enough tunes already. :-)

Corrected manual sorting in TargetLibraryInfo.def as per comments.

In D53927#1400072, @bryanpkc wrote:

In D53927#1399283, @steleman wrote:

In D53927#1399263, @rengolin wrote:

Thanks Stephan, now looks good.

Did you see the comment on lgamma coming after isoc99? I wonder if that's reproducible in all systems...

Yup I saw it. But I don't get any errors here (Ubuntu 16.04 / Ubuntu 18.something) AArch64.

And it's not clear to me how that sorted alphabetical order is supposed to work. There's a bunch of other functions that come after lgamma/tgamma that aren't sorted.

For example:

/// char * __strtok_r(char *s, const char *delim, char **save_ptr);
TLI_DEFINE_ENUM_INTERNAL(dunder_strtok_r)
TLI_DEFINE_STRING_INTERNAL("__strtok_r")
/// int abs(int j);
TLI_DEFINE_ENUM_INTERNAL(abs)
TLI_DEFINE_STRING_INTERNAL("abs")
/// int access(const char *path, int amode);
TLI_DEFINE_ENUM_INTERNAL(access)
TLI_DEFINE_STRING_INTERNAL("access")

The build was fine, but when running the compiler, this assertion in lib/Analysis/TargetLibraryInfo.cpp will fail:

static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
                       ArrayRef<StringRef> StandardNames) {
  // Verify that the StandardNames array is in alphabetical order.
  assert(std::is_sorted(StandardNames.begin(), StandardNames.end(),
                        [](StringRef LHS, StringRef RHS) {
                          return LHS < RHS;
                        }) &&
         "TargetLibraryInfoImpl function names must be sorted");

You probably don't see it because you are only building in Release mode.

P.S. The examples you listed are sorted (_ is less than a).

In D53927#1399318, @rengolin wrote:

In D53927#1399306, @steleman wrote:

Just did a rebuild after moving lgamma after isoc99. No warnings, no errors, and no difference in my test results ...

It seems like a build issue (or a different library issue), that we can fix after commit.

LGTM, thanks!

In D53927#1399263, @rengolin wrote:

Thanks Stephan, now looks good.

Did you see the comment on lgamma coming after isoc99? I wonder if that's reproducible in all systems...

In D53927#1399263, @rengolin wrote:

Thanks Stephan, now looks good.

Did you see the comment on lgamma coming after isoc99? I wonder if that's reproducible in all systems...

Reverted change in TargetLibraryInfo.cpp re: exp10|exp10f|exp10l on Linux with GLIBC.

Addressed comments from Renato:

Updated changeset to reflect comments/questions from Renato.

Added inline comments/answers to Renato's questions.

In D53927#1397790, @rengolin wrote:

In D53927#1397785, @shibatch wrote:

tgamma stands for "true gamma", and it is the function name specified in the ANSI C standard.

Hm, if this is the same as tgamma, why is it __gamma_r_finite and not __tgamma_r_finite?

Hi Renato,

Hi Renato,

This is a small - but signifcant - update to the original changeset.

This is a significant update to the original version of the SLEEF
changeset.

Thank you Hal and Renato.

In D53927#1352433, @rengolin wrote:

Perhaps best to ping Clang's commit, as that's the review that is blocking this one.

As long as we agree on the behaviour in Clang, this patch should be fine.

Yes, I know, everyone was away for the holidays. :-)

Yes, I realize people were on holidays. :-)

Removed spurious patch for an unrelated change.

Updated version of this changeset/patch, as per Renato's latest comments from D53927 (https://reviews.llvm.org/D53927).

Updated version of the patch/changeset, as per comments from Renato:

In D53927#1328001, @shibatch wrote:

The RFC for reimplementing -fveclib with OpenMP is titled "OpenMP" because the corresponding code for ARM HPC compiler requires OpenMP.
However, the proposal in RFC does not require OpenMP, as quoted below.

The new proposed #pragma directive are:

1. #pragma veclib declare simd.
2. #pragma veclib declare variant.

Both directive follows the syntax of the declare simd and the
declare variant directives of OpenMP, with the exception that
declare variant is used only for the simd context.

In D53927#1327983, @shibatch wrote:

A header file that wasn't there before is now automagically included, and there's nothing they can do to not include it. Boom! - source code change. Happy re-certification planning.

@steleman Could you explain a little bit about when a re-certification is required?
Is it required when a standard header file(e.g. math.h) is changed?

In D53927#1327091, @rengolin wrote:

Which means that those environments who will want to use SLEEF will have to build their own.
The upside is that SLEEF is likely of interest only to the rarefied world of HPC/Super-computing. Those environments can definitely build their own SLEEF and maintain their own binary copy.

The signal I'm getting from all labs (in US, Europe and Asia) is that they really *don't* want to maintain their own binaries. They just cope with it because we don't do a good job at the tools/packaging level.

I'm not willing to add more chaos into the mix.

In D53927#1327040, @shibatch wrote:

I am taking it on faith that SLEEF won't keep changing their mangling and ABI.

The name mangling rule in SLEEF is basically the vector function ABI for AArch64.
The functions for Intel ISA conform to the corresponding ABI for x86.

https://developer.arm.com/products/software-development-tools/hpc/arm-compiler-for-hpc/vector-function-abi

Hi Renato,

In D53927#1307070, @chandlerc wrote:

In D53927#1304262, @fpetrogalli wrote:

@joel_k_jones , @steleman

In D53927#1301191, @fpetrogalli wrote:

Hi @steleman , thank you for your patience.

Ping!!!

If there are no more comments directly related to this changeset, can we move this along?

1. @steleman I don't understand some of the values in your benchmarks. In particular, sin and cos should have similar timings, not differ so much as in your report. I wonder whether the choice of the CLOCK_PROCESS_CPUTIME_ID might have caused this. I think that CLOCK_PROCESS_CPUTIME_ID might translate in a syscall, and therefore cause much overhead in the measurement. I'd rather use CLOCK_MONOTONIC. Also, to make sure you are just measuring the function latency, I think you should invoke the benchmark on array of smaller size, and invoke the call a couple of times before actually starting the time measurement, to reduce the amount of noise causes by warm up effects.

Ping!!

• Reverted to using the non _u35 SLEEF function names as per comment from @shibatch.

In D53927#1288314, @shibatch wrote:

I am sorry for not reading the comments carefully, but I think 3.5-ULP functions should be used only when a fast-math option is specified.

• changed the -fveclib=<X> argument value to 'sleefgnuabi'.
• added atan2 and pow.
• spreadsheet with comparison between libm and sleef is here:

https://docs.google.com/spreadsheets/d/1lcpESCnuzEoTl_XHBqE9FLL0tXJB_tZGR8yciCx1yjg/edit?usp=sharing

• comprehensive test case in C with timings is here:

https://drive.google.com/open?id=1PGKRUdL29_ANoYebOo3Q59syhKp_mNSj

• changed the -fveclib=<X> argument value to 'sleefgnuabi'.
• added atan2 and pow
• spreadsheet with comparison between libm and sleef is here:

https://docs.google.com/spreadsheets/d/1lcpESCnuzEoTl_XHBqE9FLL0tXJB_tZGR8yciCx1yjg/edit?usp=sharing

• comprehensive test case in C with timings is here:

https://drive.google.com/open?id=1PGKRUdL29_ANoYebOo3Q59syhKp_mNSj

In D53927#1285092, @shibatch wrote:

Is it easy to add pow and atan2?
Since these functions are relatively frequently used, I think people would appreciate if you could also add these two functions.

In D53927#1282601, @llvm-commits wrote:

In D53927#1282380, @fpetrogalli wrote:

Hi @steleman , thank you for working on this!

Before doing a detailed review, I have a couple of comments and requests.

The corresponding Clang changeset is: https://reviews.llvm.org/D53928.

Added unit test case for Cavium processors - T99 and T88.

In D51939#1241238, @fhahn wrote:

LGTM. Could you please add a unittest similar to the one added in D40985?

Le Ping!

Updated per latest comments.

Updated per latest comments:

I am not sure I follow. I think @MatzeB 's issue was using getProcFamily() and it should be fine to just check the subtarget feature in enableAggressiveFMAFusion. @MatzeB summarized some benefits of making it a subtarget feature here: https://reviews.llvm.org/D40177#936974

Ping!

In D40694#970457, @fhahn wrote:

Could you please prefix the title of the commit message with [AArch64]?

In D40694#970346, @fhahn wrote:

Sorry I missed this one! LGTM, assuming you are happy with effect on ThunderX2!

Ping!

Updated diff with latest changes:

Updated large test case to use --check-prefix={CHECK-FMA|CHECK-GENERIC}.
Included Florian Hahn's small test case for FMA.
Metadata in the large LLVM IR test case is required.

In D40695#941589, @efriedma wrote:

128 is a really big number for LoopMicroOpBufferSize.

In D40696#941537, @MatzeB wrote:

My points from D40177 still stand.

Superseded by:

In D40177#935813, @fhahn wrote:

In D40177#932950, @steleman wrote:

Also if I understand correctly, this should have an impact on scheduling instructions using WriteAtomic, like CASB.

It doesn't. Maybe it was supposed to, in theory, but in reality it makes no difference whatsoever.

The only noticeable difference is in instruction cost: when Unsupported == 1, the estimated instruction cost is higher than when Unsupported == 0. Which makes no difference whatsoever in practice, given that the real cost of LSE instructions is pretty high anyway.

We've been emitting LSE instructions with no problems for many months. Whether or not Unsupported was 0 or 1 made no difference whatsoever.

It is probably unlikely that atomic instructions are in hot loops of most benchmarks, so I am not surprised that you do not see any impact on runtimes (assuming that's what you meant with 'it makes no difference whatsoever). However there should be cases where it should make a difference when scheduling, but it probably requires some time to come up with a test case.

Please avoid getProcFamily() checks outside of AArch64SubtargetInfo. Use target features and transfer the logic into SubtargetInfo!

Also if I understand correctly, this should have an impact on scheduling instructions using WriteAtomic, like CASB.

Added test case.

In D40177#928936, @mgrang wrote:

Could you please add a few tests? Also please remember to add llvm-commits as subscriber to your patches.

Restored definition for defm atomic_load_nand in TargetSelectionDAG.td,
which I had removed by mistake.

In D35319#831965, @christof wrote:

Thanks for the hard work. It looks good. Quite an exhaustive list of tests as well.

Updated changeset to use LDOPregister_patterns.
Implements all the LD<OP> LSE Atomics with the exception of NAND.
Updated test in atomic-ops.lse to cover all memory ordering models.

I have uploaded the LLVM patch that exhibits this problem here:

It does not appear that the multiclass design you are advocating here does what we'd expect it to do.

Progress update: updated with the latest changes.

In D35319#808812, @t.p.northover wrote:

In D35319#807963, @t.p.northover wrote:

This diff covers lots of different areas:

Updated and corrected AArch64DeadRegisterDefinitions::ShouldSkip.