[PATCH] D56593: [SelectionDAG][RFC] Allow the user to specify a memeq function (v5).

Sat Nov 6 16:12:49 PDT 2021

goldstein.w.n added a comment.

In D56593#3113746 <https://reviews.llvm.org/D56593#3113746>, @goldstein.w.n wrote:

> In D56593#3113727 <https://reviews.llvm.org/D56593#3113727>, @MaskRay wrote:
>
>> In D56593#3113701 <https://reviews.llvm.org/D56593#3113701>, @goldstein.w.n wrote:
>>
>>> In D56593#1393325 <https://reviews.llvm.org/D56593#1393325>, @jyknight wrote:
>>>
>>>> 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 <https://marc.info/?t=163157542200002&r=1&w=3>.
>>>
>>> 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 <https://sourceware.org/git/?p=glibc.git;a=commit;h=44829b3ddb64e99e37343a0f25b2c082387d31a5>
>>> 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 <https://gitlab.com/cpu-rt/glibc>.

Do you know if there is any way to check for the `__memcmpeq` declaration? We put it in `string.h` particularly because the issue of how to know when this optimization is valid came up when making the proposal.

As well, does having the optimization attached to the declaration make sense? It may still need to be attached to `isGNUEnvironment` in case another libc implementation has `__memcmpeq` with slightly different semantics.

>> The glibc 2.35 availability does not make this optimization exploitable, at least for few (5+?) years :)
>
> Indeed :/
>
>> One option is to hide such transformation under a `cl::opt` option.
>
> Sorry not sure I understand, can you explain?

See what you mean and why it might be necessary.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56593/new/

https://reviews.llvm.org/D56593