Also see D24480 and the list of proposals and commits mentioned there.

In D45862#1074819, @mkazantsev wrote:

For now, my point is the following: yes, we are *already* doing all this bit magic. Yes, it looks like miscanonicalization, and we can discuss getting rid of all such transforms at all. But since we are already doing it and haven't yet decided to clear it off, let's at least do it well. Once we decide that all this stuff is counter-canonical, we can happily remove all this. What do you think of that?

LGTM, but please do check in the tests with baseline output first (just in case the code change is reverted, we don't want to lose the tests).

In D45563#1074387, @lebedev.ri wrote:

In D45563#1074051, @spatel wrote:

LGTM. You could increase diversity in the constant mask tests by not using a single-string-of-set-bits constant (eg 0x0f0f0f0f instead of 0x0000ffff).

But since we don't do anything (not fold, not unfold) with these patterns
with constant masks, is there any point in adding even more tests?

In D45862#1074397, @lebedev.ri wrote:

In D45862#1073841, @spatel wrote:

I don't think we should be doing any of these select-of-constant transforms in instcombine.

It's worse for code analysis, more IR instructions, and may be detrimental to perf. Think about the cases where a conditional move executes at the same speed as a simple add (Ryzen?) or we have profile data for the compare, so branch prediction is perfect.

There's lots of code that does this kind of thing in the DAG, and that's where I think it belongs (using target hooks as needed). There was some discussion about this on llvm-dev here:
https://groups.google.com/forum/#!topic/llvm-dev/pid_thv2X-A

So I think we should be removing some of these transforms from instcombine rather than adding to them.

There is a second side to this though.

Even if all such "performance-degrading" transforms are removed from instcombine
(yes, i think the pass is rather huge and monolithic, this is a problem), it won't solve the problem.
The same 'bad' patterns surely could be produced via some other way.

I don't see how this is any different than lowering intrinsics.
Of course, they are no longer intrinsics, but IR, so optimizations may
break their canonical form, and backend will need to be adjusted
to recognize the IR patters (and potentially recognize the patterns
that did not originate form intrinsics, which is great).

So i'd say this really is backend's (DAGCombine?) problem.
This is just a question of whether or not there is interest to have a high quality codegen,
regardless of the input. It should recognize the 'bad' patterns, and if profitable,
transform to improve 'performance'. In this case, back to select.

And yes, absolutely, this is more complicated than just "stop dealing with it in instcombine" :)

LGTM. You could increase diversity in the constant mask tests by not using a single-string-of-set-bits constant (eg 0x0f0f0f0f instead of 0x0000ffff).

I don't think we should be doing any of these select-of-constant transforms in instcombine.

In D45653#1073189, @sorokin wrote:

$python2 update_llc_test_checks.py --llc-binary <directory-of-llc>/llc llvm/test/CodeGen/X86/sibcall.ll

and it modified the sibcall.ll. I'm not familiar with with the utility. I'm afraid I don't know how to use it properly and what is the meaning of the diff. I attached 2.patch. It is the changes this tool does to sibcall.ll. {F5984860}

I don't think we have what you're describing (looking through clang source
because I don't think there's any actual documentation...).

That looks ok, but I'm curious;

1. How do you discover this problem?
2. Is it possible to make the problem visible in a regression test (if so, please add a test)?
3. The DenseMap usage is based on the code in BypassSlowDivision - does it have the same problem/fix?

In D45418#1072571, @xbolva00 wrote:

Now you can merge this patch, @spatel

I see that patches were applied to the Chromium sources:
https://bugs.chromium.org/p/chromium/issues/detail?id=831145#c59

LGTM.
I've never touched a sparc test myself, but this looks like a reasonable copy/paste. :)
Feel free to make any sparc regex changes needed to improve those CHECK lines.

In D45733#1071068, @lebedev.ri wrote:

I don't think it will be possible to check that until after the instcombine part has landed, so ok, at least for now i will stop unfolding [constant mask] in dagcombine.

While there, any hint re pattern matchers for this code?

In D45733#1071005, @lebedev.ri wrote:

Yeah, that is the question, i'm having. I did look at mca output.

Here is what MCA says about that for -mtriple=aarch64-unknown-linux-gnu -mcpu=cortex-a75

diff.txt1 KB

Download

Or is this a scheduling info problem?

See my comment in D45733. Sorry that I didn't get a chance to review/discuss this sooner.

If the mask is constant, right now i always unfold it.

The patch doesn't compile as-is. Please post a corrected patch.

Thanks for coming back to this. I think this patch is fine as one more match of a potential IR variant, but...
Since the time we started the discussion in D41353, there was a proposal to improve matching for ctpop and ctlz in D45173. I'm curious what others think about the fact that we match bswap/bitreverse in regular instcombine. Would it be better to match all of those intrinsics in the same place? Do we distinguish these based on how often we expect them to occur?

In D45563#1069968, @nemanjai wrote:

In D45563#1069916, @spatel wrote:

Not sure this answer is pertinent any longer, but on PPC we can generate the VSX version of the vector select (xxsel) for rather specific situations. See test/CodeGen/PowerPC/vselect-constants.ll and test/CodeGen/PowerPC/vsx.ll.

In D45563#1069922, @lebedev.ri wrote:

In D45563#1069916, @spatel wrote:

Don't know what changes are planned here, but this is on the right track. We want to have coverage of the possible canonical IR variations for various targets.

I'm working on that right now, got it working, maybe will update this + post the dagcombiner part (that is where i should have put it, right?) in a few hours.

Don't know what changes are planned here, but this is on the right track. We want to have coverage of the possible canonical IR variations for various targets.

I don't understand what's going on here. Why is this patch marked 'Accepted'? In general, you shouldn't approve your own patches that you've posted for review.

Unfortunately when this if was added, no explanation was given why it is needed here. The corresponding test case is marked rdar://7726868, but I can not see what it is about.

Thanks for working on this. Adding some more potential reviewers...

Note that for the value tracking improvement, there are a couple of other passes (that I've never looked at) that use haveNoCommonBitsSet(). If it's possible to show a test improvement from this change for those passes, that would be nice.

LGTM - see inline for a couple of potential follow-ups.

In D45598#1067909, @nemanjai wrote:

From the PPC perspective, this is fine since the sequences are equivalent. It also seems perfectly reasonable to look through a free operation here.

Ping * 2.

In D45453#1066811, @wristow wrote:

I've updated the code-change in "InstCombineAddSub.cpp" to require both 'reassoc' and 'nsz' to do the transformations. (I haven't changed the name from FAddCombine to FAddSubCombine or ReassociateFAddFSub. I could do that if we move forward with this.)

There could be some kind of symbiotic behavior here, but this is really saying that we don't have a well-defined division of labor between these passes, right?

Yes, a lack of a well-defined division of labor between these passes is exactly my concern. I'll look into adding or modifying tests. Depending on possible interaction, that may not be fruitful. Ultimately, that should be addressed.

In looking this over, I was pleasantly surprised that I didn't run into the troublesome interaction I was concerned about. So I've added new tests to "InstCombine/fast-math.ll" to check for the transformations happening when 'reassoc' + 'nsz' are on (with only '-instcombine' used). I've also added tests that verify that 'reassoc' alone doesn't enable the transformation in the appropriate places. Lastly, I updated the tests I had previously modified to use both 'reassoc' and 'nsz' where appropriate when testing for the minimal set of needed flags to transform things.

LGTM.

Side note: this patch / commit should not be labeled 'NFC'.

I think the patch is correct, but it's hard to keep all of the values straight (because there are a lot!). What do you think about rearranging it a bit like this and using m_c_And():

In D44909#1065786, @efriedma wrote:

After this change, is there some way to express this approach in C?

You can use platform-specific intrinsics, e.g. _mm_cvtsd_si32 is defined to return INT_MIN for out-of-range values. Or, in portable C, you can do a range check: if (x <= INT_MAX && x >= INT_MIN && (double)(int)x == x).

In D44909#1065673, @thakis wrote:

We think there's an UB check for this already, but v8 isn't UB clean.

The fast way to do the "is this double an int" check on an assembly level is to convert to int and back and then compare if the roundtripped value is equal. After this change, is there some way to express this approach in C?

Reopening - reverted at rL329920.

In D44909#1065600, @hans wrote:

In D44909#1065109, @spatel wrote:

In D44909#1065089, @thakis wrote:

This broke some large integration test on arm64 in Chromium (https://crbug.com/831145 – doesn't yet have any information other than "something is broken" and that we bisected it to this revision). It sounds like the patch got rewritten during review; does "formally allowed by the standard, but extremely likely to break things and surprise people" still apply? It sounds like this is supposed to be behavior-preserving for values that aren't NaN and Inf, but it does change behavior for those two? (If so, in what way?)

The patch started off as an IR transform, and the quoted comment applied only to cases (I hope) where the target doesn’t have native support for an fptrunc instruction. NaN/inf inputs are undefined with these ops, so if something was relying on that undefined output, that could be the source of the problem.

I’m not an arm64 expert, but the tests show that we should codegen ‘frintz’ instructions with this patch. So that would be the place to look for diffs.

We found it: https://bugs.chromium.org/p/chromium/issues/detail?id=831145#c48

It's exactly the "formally allowed by the standard, but extremely likely to break things and surprise people" situation :-) The question is just what the appropriate fix is...

In D44909#1065089, @thakis wrote:

This broke some large integration test on arm64 in Chromium (https://crbug.com/831145 – doesn't yet have any information other than "something is broken" and that we bisected it to this revision). It sounds like the patch got rewritten during review; does "formally allowed by the standard, but extremely likely to break things and surprise people" still apply? It sounds like this is supposed to be behavior-preserving for values that aren't NaN and Inf, but it does change behavior for those two? (If so, in what way?)

LGTM. I think you have a good understanding of how to write/check instcombine tests, so feel free to add those as needed without pre-commit review unless you're not sure about something.

In D45378#1060270, @xbolva00 wrote:

In D45378#1060239, @spatel wrote:

This is pushing instcombine beyond local simplifications. Propagating values across blocks should probably be handled in CVP (-correlated-propagation).

Maybe, but here it also fits, every info required for this transformation is available here.

In D45453#1063950, @wristow wrote:

A couple of short comments/questions, and one longer one:

In D45453#1063225, @spatel wrote:

... the remaining big blob is FAddCombine() - which is poorly named because as shown in this patch, it's also called from visitFSub()...

Should we rename it to FAddSubCombine() in this work (assuming this patch moves forward, given the other questions here)?

Patch updated:
Fixed the call to dominates() to use the phi's block, not the phi itself.

The tests here demonstrate the overlap between instcombine and reassociate, and based on feedback I've gotten, I'd say that rL170471 was a mistake for instcombine. It created a mini-reassociation pass within instcombine.

In D45393#1062223, @FarhanaAleen wrote:

On the other hand, this transformation can be implemented in a cleaner way with 1) (inside InstCombine) under the same consideration(target checks) with a simple extension. 1) also allows us to reuse the existing implementation. Another benefit of 1) is it will contain all the relevant scalarization code in one place. The only con is it requires additional support for certain targets; X86 at least and may be more.

LGTM - see inline for a nit.

Patch updated:

1. Move the dominance check, so it only happens once. This matches the logic in InstSimplify that handles the simpler case of a phi where all incoming values are identical except for undefs.
2. Added a debug statistic to count how often this fires.

In D45448#1062055, @efriedma wrote:

Any idea how often this triggers in practice?

LGTM (assuming the backend gets this right in all cases and we have tests for that).

In D45421#1061875, @craig.topper wrote:

Yes. I'll make the llvm changes before committing this. Just wanted to make sure this direction was ok first.