Page MenuHomePhabricator

goldstein.w.n (Noah Goldstein)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 6 2021, 11:04 AM (33 w, 1 d)

Recent Activity

Thu, Jun 16

goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

What do you think about just using the clang AST and checking if there is a valid definition for __memcmpeq + isGNUEnvironment?

And if there is no declaration and somebody wrote “__builtin_memcmp(…) == 0” ? We sadly cannot use memcmpeq then, no?

Thu, Jun 16, 3:44 PM · Restricted Project, Restricted Project

Wed, Jun 15

goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

Seems fine as well.

Maybe there should be a RFC at Discourse ? ..

Wed, Jun 15, 2:44 PM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

We already effectively have a platform version number on Mac/iOS/Android; adding a version number for another platform isn't a big deal. (Yes, it doesn't really scale in the sense that every libc needs to have some entries in TargetLibraryInfo.cpp, but we basically end up doing that anyway, in practice.)

The tricky thing is that there isn't any easy way to tell the glibc version number. The clang driver would have to dig through either system headers or the glibc library itself.

Wed, Jun 15, 2:14 PM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

Maybe new flag like -fglibc-version=ver ?

It could nicely fit as we already have -fbinutils-version and -fgnuc-version.

Wed, Jun 15, 10:36 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

Wouldn't that almost definitionally only work if the file was being compiled statically? Otherwise you would
run into the issue that compiling on a system with glibc >= 2.35 produce an executable that wouldn't work
on a system with glibc < 2.35

There is absolutely no expectation of compatibility in that direction. A binary compiled against version X of glibc can run against any glibc version Y, where Y >= X -- but NOT the other way around.

Wed, Jun 15, 10:17 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

Ideally LLVM should just "enable" memcmpeq if glibc version is > XYZ. Not sure if there is a realiable way to do it. @efriedma ?

Wed, Jun 15, 10:09 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.
Wed, Jun 15, 9:44 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

@courbet Any followup as we have got "'__memcmpeq" ?

I was under the impression that bcmp was redirecting to __memcmpeq when the latter was available. Isn't that sufficient ?

It is not.

This was rejected for the same reasons the bcmp optimizations where rejected. bcmp is not standardized and since
glibc has been maintaining it as a true memcmp (that can be used for sort), there is not reason for users to not have
begun to rely on that behavior.

This is kinda sad, but OK.

Indeed. The original patch that kicked this off was just to optimize bcmp so the LLVM optimization
would be meaningful. Cest la vie.

I don't think we can call __memcmpeq directly because this will not work on older versions of the glibc (as discussed above).

I have a patch here: https://reviews.llvm.org/D127461

I personally don't have a strong opinion, but I know that during the initial RFC some people were against adding a backend flag for this (https://reviews.llvm.org/D56248): see the discussion here: https://lists.llvm.org/pipermail/llvm-dev/2019-January/128827.html.

Wed, Jun 15, 8:50 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

@courbet Any followup as we have got "'__memcmpeq" ?

I was under the impression that bcmp was redirecting to __memcmpeq when the latter was available. Isn't that sufficient ?

Wed, Jun 15, 8:20 AM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

@courbet Any followup as we have got "'__memcmpeq" ?

Wed, Jun 15, 8:17 AM · Restricted Project, Restricted Project

Sat, Jun 11

goldstein.w.n added reviewers for D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp: bogner, jyknight.
Sat, Jun 11, 5:07 PM · Restricted Project, Restricted Project

Thu, Jun 9

goldstein.w.n added a comment to D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.

NB: Also have patch to add __memcmpeq as bonafide builtin in clang. Figure that's a bigger change but if people are interested LMK.

Thu, Jun 9, 5:14 PM · Restricted Project, Restricted Project
goldstein.w.n requested review of D127461: [SelectionDAG] Use __memcmpeq to replace bcmp and bool usage memcmp.
Thu, Jun 9, 5:09 PM · Restricted Project, Restricted Project

Nov 6 2021

goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

It'd be great to see this somewhat more widely publicized, outside of just the clang community. If libc implementors are aware of the gains and are willing to provide an actually-faster bcmp implementation, it'd be a lot better, than having this optimization that doesn't really optimize anything without users providing their own bcmp implementation.

Tried to add an optimized bcmp for GLIBC here.

It was not well received because bcmp is not a standard function. It seems GLIBC supports bcmp out of reluctant necessity rather than any desire for it to be fast.

There was agreement that the functionality was useful so GLIBC landed on __memcmpeq'to get the functionality. The patches made it to HEAD
and will be available starting with the 2.35 release. It declared in "string.h" or can be queried with GLIBC version >= 2.35.

Currently only x86_64 has an optimized version, the rest of the targets still just redirect to memcmp.

Working on a patch to add support in LLVM.

I understand and accept the viewpoint that: for glibc, bumping the symbol version for bcmp may lead to difficuly-to-debug issues when users try to upgrade glibc.
An ABI-only symbol in the reserved namespace looks good to me.

AFAIK while LLVM has Triple::isOSLinux and Triple::isGNUEnvironment and there are optimizations dispatching on linux-gnu there is no way detecting the version. (well, Apple platforms and some other OSes have such checks)
The optimizations just assume that a very old glibc version may be used (perhaps 2.10+ or early 2.20+).
In addition, there are many cross compiling users where no version is specified.

I was planing to add Triple::isGNUVersionLT and use it in a function hasMemcmpeq in a similar vein to hasBcmp. Will there be an issue with that?

So the version parsing doesn't appear to work out of the box for GLIBC and your right that it will miss cases (cross compilation as you pointed out) as well as free standing compilation or libraries like cpu-rt.

Nov 6 2021, 4:12 PM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

It'd be great to see this somewhat more widely publicized, outside of just the clang community. If libc implementors are aware of the gains and are willing to provide an actually-faster bcmp implementation, it'd be a lot better, than having this optimization that doesn't really optimize anything without users providing their own bcmp implementation.

Tried to add an optimized bcmp for GLIBC here.

It was not well received because bcmp is not a standard function. It seems GLIBC supports bcmp out of reluctant necessity rather than any desire for it to be fast.

There was agreement that the functionality was useful so GLIBC landed on __memcmpeq'to get the functionality. The patches made it to HEAD
and will be available starting with the 2.35 release. It declared in "string.h" or can be queried with GLIBC version >= 2.35.

Currently only x86_64 has an optimized version, the rest of the targets still just redirect to memcmp.

Working on a patch to add support in LLVM.

I understand and accept the viewpoint that: for glibc, bumping the symbol version for bcmp may lead to difficuly-to-debug issues when users try to upgrade glibc.
An ABI-only symbol in the reserved namespace looks good to me.

AFAIK while LLVM has Triple::isOSLinux and Triple::isGNUEnvironment and there are optimizations dispatching on linux-gnu there is no way detecting the version. (well, Apple platforms and some other OSes have such checks)
The optimizations just assume that a very old glibc version may be used (perhaps 2.10+ or early 2.20+).
In addition, there are many cross compiling users where no version is specified.

I was planing to add Triple::isGNUVersionLT and use it in a function hasMemcmpeq in a similar vein to hasBcmp. Will there be an issue with that?

Nov 6 2021, 1:12 PM · Restricted Project, Restricted Project
goldstein.w.n added a comment to D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5)..

It'd be great to see this somewhat more widely publicized, outside of just the clang community. If libc implementors are aware of the gains and are willing to provide an actually-faster bcmp implementation, it'd be a lot better, than having this optimization that doesn't really optimize anything without users providing their own bcmp implementation.

Nov 6 2021, 11:26 AM · Restricted Project, Restricted Project