In D68930#1715680, @lebedev.ri wrote:

In D68930#1715645, @spatel wrote:

In D68930#1715209, @lebedev.ri wrote:

Ignoring the -O3 issue, i'm not really confident AggressiceInstCombine is a great fit for these problems.
The thing is, these folds are not one-off "folds - yay, no fold - shrug".
There isn't much interest in this fold happening in itself.
The really important part is that it will pave the road for other folds to happen.
In particular, this fold will allow e.g. rL373964 to happen.
The situation is likely similar with all other "aggressive" folds i'we added.

Yes, I understand this patch alone doesn't look like a big change.

To recap, the main problem is not that they use InstSimplify,
but that SimplifyAssociativeBinOp() uses recursive reasoning, correct?

In D68930#1715209, @lebedev.ri wrote:

Ignoring the -O3 issue, i'm not really confident AggressiceInstCombine is a great fit for these problems.
The thing is, these folds are not one-off "folds - yay, no fold - shrug".
There isn't much interest in this fold happening in itself.
The really important part is that it will pave the road for other folds to happen.
In particular, this fold will allow e.g. rL373964 to happen.
The situation is likely similar with all other "aggressive" folds i'we added.

In D69127#1715640, @spatel wrote:

This patch would already have hoisted the zext ahead of the i32 ctpop in that basic example, but if we had started with this i32 code, then we'd widen to i64 ctpop because we assume it's no more expensive than i32 ctpop+zext. For the PPC examples, the zext becomes a mask op, so we eliminate that instruction.

In D69127#1715373, @craig.topper wrote:

So the assumption here is that the popcnt we choose here is the same performance as any smaller popcnt that LegalizeDAG would pick through promotion. Or near enough that its better than having 2 zexts?

In D68930#1714878, @lebedev.ri wrote:

In D68930#1714856, @xbolva00 wrote:

sadly i'm still seeing regressions (although smaller!) in target benchmark, and analysis of IR points to *this* missing pattern

But does this or base pattern shows up somewhere else (test suite/clang/chromium) than in your benchmark?

It's not "'your benchmark'", that is a real code. I'm not pulling it out of a thin air.

In D69127#1715078, @jsji wrote:

LGTM. Thanks.
Do we want to consider any_extend as well?

LGTM - see inline for a minor improvement.

In D69044#1714609, @davezarzycki wrote:

In D69044#1714314, @spatel wrote:

Can we make the x86 change to combineVectorSizedSetCCEquality() independently and before the change to TargetLowering?

Given that the previous attempt was reverted because of perf only it would be good to show some perf data here in the proposal. Micro-benchmark or more substantial. cc'ing @courbet in case there's already a test harness in place for that.

I haven't looked at this in a while, so I wonder if we now have the infrastructure within memcmp expansion to create the partial vector code with 'ptest' shown here:
https://bugs.llvm.org/show_bug.cgi?id=33914

Hi @spatel,

Thanks for the feedback and yes we can separate the changes. A few thoughts:

1. The inlined memcmp is much smarter than the Glibc memcmp code these days, at least for pure equality comparisons. In particular, the compiler's overlapping load optimization is really nice (see D55263).
2. This change proposal intentionally sidesteps more complex memcmps where the return result is tristate (greater, lesser, or equal), not binary (equal versus not). The tristate memcmp is what regressed in PR33914.
3. It would be easy to contrive a microbenchmark that makes any libc memcmp look very bad and the inlined memcmp look very good. This would be fun, but not informative or actionable.
4. If I were to design a somewhat interesting and quasi-realistic micro-benchmark, I might create a carefully crafted test that hammers on a llvm::StringSwitch where the cases need more than two load pairs to be inlined.
   
   This all being said, and if I might be totally honest, I'd like to observe two things:
5. The size of inlined memcmps tend to have a log-normal distribution with a small mean/median/variance. In other words, the vast majority of inlined memcmps (especially on AVX or AVX512 CPUs) don't need more than two load pairs.
6. That being said, the specialization that a higher max load pair count allows matters more on simpler CPUs with smaller vectors (if at all) and fewer micro-architectural tricks to mask the cost of libc's dynamic dispatch.
   
   Therefore, I would argue that the max load pair count should be derived, not fixed. For example, I think the following psuedo-code would yield reasonable results across the semi-recent history of Intel's product line: 2 * CACHELINE_SIZE / PREFERRED_VECTOR_SIZE
   
   I'd further argue that the compiler shouldn't assume that "max load pairs per block" being less than "max load pairs" is predictable by the branch predictor, but that's a separate discussion.
   
   Your thoughts would be appreciated. Thanks!

In D68930#1713151, @lebedev.ri wrote:

Any thoughts on this?

In D69161#1714549, @foad wrote:

In D69161#1714388, @spatel wrote:

I think this patch is also allowing the following constructs, but not testing for them.

Are there any existing tests for fast math flags on select or phi, that I could extend? I didn't even know they were allowed because they weren't mentioned in this part of the docs (https://llvm.org/docs/LangRef.html#fast-math-flags) and they aren't explicitly listed in the implementation of FPMathOperator::classof.

LGTM

LGTM

The LangRef needs edits for this change.
I think this patch is also allowing the following constructs, but not testing for them.

Also worth mentioning: one of the suspects for the regression in PR33914 was a trailing cmov. That's gone now*, so we might want to implement the simpler fix (expand everything to 4) and re-check perf.

Can we make the x86 change to combineVectorSizedSetCCEquality() independently and before the change to TargetLowering?

LGTM

LGTM

LGTM with minor fixes as suggested.

In D68982#1711031, @lebedev.ri wrote:

You need to work on svn trunk.

Patch updated:
Change implementation to a pure SLP bailout based on pattern-matching of a load-combine-reduction opportunity. Same test results.

I didn't step through the entire patch/tests, but the first few examples are flattened/simplified by -simplifycfg. Would it be better to look there to add the missing functionality? Is there a test in this patch that shows a case that simplifycfg can't already flatten?

Also, I have no knowledge of MSP430, and I don't see a code owner for this target. cc @asl @krisb based on D54623.

I understand the direction, and I think there's general agreement on it (adding some more potential reviewers based on the bug report and llvm-dev thread), but:

1. Add the tests in the patch description to a file in test/CodeGen/MSP430 as an NFC commit *before* applying this patch. Generate baseline test assertions for current asm using utils/update_llc_test_checks.py.
2. Split this patch up. In the 1st step, we can add the TLI hook and 1-2 usages of that hook that show a test diff for MSP430. Ideally, we can add small tests for each proposed usage of the TLI hook.

Reverted at rL374851 - appears to cause a test-suite failure and stage 2 failure.

Patch updated:
Rebased after committing baseline tests and cosmetic changes to the code.

Commandeering to rebase.

Patch updated:
Added code comment about the inverted/commuted variations of these patterns.

I haven't tested this on trunk, but I added the DAGCombiner reversals:
rL374397
rL374555
...so this should be good to go. Should I commandeer?

@joanlluch - this is a case where we could transform incoming shift to select and benefit small targets as you've noted.

There seems to be general agreement that this is a temporary (stopgap) solution until we can do limited load combining in IR. So it's a question of whether we're ok with a cost model hack to overcome the motivating bugs while we figure out how to add a new pass.

Note that @reames suggested that we should publicize the previous patch to make sure there were no known unintended consequences from this change, so:
http://lists.llvm.org/pipermail/llvm-dev/2019-September/135444.html

LGTM - I applied the patch locally and ran 'make check' and don't see failures now.

In D68706#1701915, @jdoerfert wrote:

Can you add the test I provided as well?

Patch updated:
No diffs in this patch itself, but rebased after adding test at rL374190.
I don't have much experience with addrspaces or inbounds, so let me know if I should change anything else.

LGTM - you may want to annotate the title with 'NFC' when committing to indicate this patch doesn't actually change code.