Page MenuHomePhabricator

[libcxx]: Unroll std::find on AIX
AbandonedPublic

Authored by etiotto on May 6 2021, 10:05 AM.

Details

Reviewers
jasonliu
xingxue
Mordante
ldionne
Group Reviewers
Restricted Project
Summary

This patch specializes the std::find implementation on AIX to unroll the while loop in that function for RandomAccessIterators.

Diff Detail

Repository
rCXX libc++

Event Timeline

etiotto created this revision.May 6 2021, 10:05 AM
etiotto requested review of this revision.May 6 2021, 10:05 AM
etiotto added a reviewer: Restricted Project.May 6 2021, 11:28 AM
Mordante added a reviewer: Mordante. Mordante added 1 blocking reviewer(s): Restricted Project.May 6 2021, 12:47 PM
Mordante added a subscriber: Mordante.

This patch increases the maintenance burden for libc++, so before accepting this patch I would like to know more about the benefits it brings.
Can you provide some benchmarks for these changes?
Why would these changes only be beneficial for AIX and not for other platforms?

ldionne requested changes to this revision.May 7 2021, 11:09 AM
ldionne added a subscriber: ldionne.

Echoing @Mordante 's feedback here - we will either take this patch for everyone if it benefits everyone, or not take it at all. If you do some benchmarking and are able to show that it's an improvement (and is worth the added complexity + code size), then I'd be OK with making this change for everyone. But it has to be backed.

This revision now requires changes to proceed.May 7 2021, 11:09 AM

Just curious, why you cannot use:

#if defined(AIX)
#pragma unroll 4
#endif
for(….)

it seems like we dont specialize std::find to use memchr if possible?

Echoing @xbolva00's comment, why is compiler's hand being forced here in the first place?
I'm disheartened by seeing such changes, because there's clearly no cost modelling here,
and it is assumed that one size fits all. If this is beneficial, then this is certainly
a LLVM bug. In particular, LoopFullUnrollPass says `will not try to unroll loop with
runtime trip count -unroll-runtime not given`, and LoopUnrollPass says:
`Bailout for multi-exit handling when latch exit has >1 predecessor.
Multiple exit/exiting blocks in loop and multi-exit unrolling not enabled!`

Fixing those instead would benefit everyone.

etiotto abandoned this revision.May 25 2021, 6:11 AM