The change should be visible in tests because there's a DAGCombine that uses isKnownNeverNaN()?

In D50791#1201420, @craig.topper wrote:

Thinking forward to a future where we have SVML library support for v4f32 and a v8f32 sin/cos. What should we do for v5f32?

Make the vector type check an assert instead of an actual condition for the transform (no test diffs).

LGTM

LGTM

In D45653#1200806, @aleksandr.urakov wrote:

@spatel Excuse me, can you explain, please, what's wrong with this patch now?

Ivan is my colleague, he have asked me to solve this review a week ago. And he have mentioned here, that he doesn't mind if someone will comandeer the review...

There was the very long delay with a reply, and I understand that it's not good (and I'm very sorry for that really). But now I want to solve this in the nearest future.

I really hope for your understanding.

I've fixed this by doing a ptrtoint convert on the assumption that ptrtoints are free.

Forgot to click 'Accept' here in Phab.

About the autogen script problem - we could hack that to use something less likely than 'TMP' as its regex string (eg, 'AUTOGEN'), but that will result in significant test file churn if someone updates an existing file.

Tests LGTM. It might help me or other reviewers see how these patterns escape the current set of transforms in foldAndOfICmps(), so please commit.

LGTM

In D50190#1195350, @xbolva00 wrote:

• Rebased/updated

I suppose the extra check doesn't hurt anything, and this is all disorganized anyway. The simplifications of calls to constants should really be in InstSimplify.
The vector changes look correct, so LGTM.

You should be able to adjust 2 lines of code to allow non-commutative ops after:
rL339439

In D50190#1194883, @xbolva00 wrote:

• Added/fixed/updated tests (ok now?)

LGTM

Ping.

I'm not finding the mails where the extraction of target-specific code came up, but my vague memory says @rnk had some ideas about how to improve things.

The title of this patch includes "Constant folding" - shouldn't the code be in ConstantFolding.cpp or some descendant of that?

LGTM

In D50195#1193315, @mcberg2017 wrote:

At this point there isn't any code either way that checks for fine...

In D50195#1192761, @mcberg2017 wrote:

Even with all the above changes, the opportunity persists. We need to move:

// fold (fsub A, (fneg B)) -> (fadd A, B)

if (isNegatibleForFree(N1, LegalOperations, TLI, &Options))
  return DAG.getNode(ISD::FADD, DL, VT, N0,
                     GetNegatedExpression(N1, DAG, LegalOperations), Flags);

after the match as it sweeps up part of the key expression, and never completes the rest, leaving it partially optimized for some cases (in the test case noted, its for zero gain). Basically, this key context should be after all the major specialization matches to give each of the others a chance to complete.

I'm still confused. I don't see why we need to check isNegatibleForFree().

Marking as accepted here on Phab to avoid unknown state when committing.

About the bit hacking: I don't think clang should be in the optimization business. We should be able to take the most obvious/simple representation for this builtin and reduce it as needed (either in instcombine or the backend). So it would be better to use the version with the subtract rather than shift/or. That version corresponds more directly to the code in APInt::getMinSignedBits()?

LGTM

In D50081#1192072, @hans wrote:

In D50081#1185480, @hans wrote:

In D50081#1185447, @xbolva00 wrote:

Maybe this fix be should merged into 7.0 too?

@hans

Sounds good to me, but it needs to be committed first obviously :-)

Merged in r339234.

As we discussed in D49306, I agree that this is probably a good perf transform, but I don't think we've shown any compelling reason to do this in IR vs. DAGCombiner.

In D50195#1191124, @mcberg2017 wrote:

I like these test versions a little better, they show an opportunity yet to be had for the IR optimizer.

I may be starting a substantial yak shaving with the inline question, but I wonder if it wouldn't be better to canonicalize with the 'not' after the 'xor' (ie, the inverse of this patch). I don't have a motivating example yet, but that just seems more natural rather than semi-arbitralily choosing an operand to invert.

Let's handle this one step at a time. Please remove the FP changes. Let's make this patch only about handling the non-commutative integer binops.