Added support for handling the nsw/nuw ssub.with.overflow intrinsic.

In D36059#828874, @chandlerc wrote:

IMO, there is no need for doing this in this place. If we're just leaving a marker here for the target to expand, we don't need to do anything. We already get a chance to custom expand the libcall in the target. Adding the versioning doesn't make that any simpler given that it still needs to introduce a loop.

The difference is that at the target codegen level we can't as easily do the predicate analysis as we can at the IR level.

If, for a particular target, it is worth emitting a versioned, carefully target-crafted loop or instruction sequence, I would expect them to not use this pass but to custom lower the calls in the backend much like x86 does for constant-size calls.

But in the patch description you say that one of the challenges is constructing *just* the right IR to get efficient codegen from the backend. I understand this is for x86 right now, but if you don't have plans to allow other targets to work well with it, why not put it into the Target/X86 directory and make it a backend-specific IR pass to avoid confusion?

I'm resigning from SVE upstreaming related activities, so Graham will be taking over this patch and others from here.

Instead of generating loop IR for the fast path, how about creating a versioned memcpy/memset with the constrained parameters guarded under the condition test? That way, in the back-end the exact preferred optimal code can be generated, allowing for unrolled loop bodies specific to individual targets.

Updated to use MatchBinaryOp.

LGTM.

Ping.

The reason it's removed is because it's not actually used anywhere, just as a default value. I'm not going to debate it further though so I've put it back in.

Ignore previous comment, was supposed to be added to D35118.

In D35118#806730, @rengolin wrote:

@jmolloy Can you check this change, please?

In D35307#806713, @rengolin wrote:

Hi Amara,

This seems a very raw change, without any further description, comments or proper usage, other than a few changes on random places.

Removing comment.

In D35264#805566, @mkuper wrote:

This looks wrong.
In particular, it would break for something like:

a = *p;
free(p)
while (k) { // k is always false
 ... = *p;
}

We have llvm::isSafeToLoadUnconditionally() that does the domination check safely, but just checking that it's safe to load unconditionally in the pre-header may not be enough.

Consider something like:

a = *p;
while (k) {
  if (m) {
    free(p);
  }
  ... = *p;

}

Now, in those cases we shouldn't be hoisting regardless, because if p escapes, the value may not be loop-invariant.
But we need to make sure the patch doesn't break that, so additional tests may be needed.

Whitespace fixes.

Ping.

Sure, up for review at D35118.

In D35076#801221, @rengolin wrote:

Hi Amara,

Thanks for the patch, looks trivial to me. I guess we don't have any targets with SVE by default, so we don't need most tests.

I'm guessing you have a Clang counterpart, too?

cheers,
--renato

LGTM.

Sorry, this fell off my radar. LGTM.

Hi all,

In D33058#752230, @Ayal wrote:

The specific a[i]+a[i+1] case could indeed be handled before vectorization, namely by (LoopSimplify?) hoisting the cast along with the load. This applies in general to other instructions that operate symmetrically on both a[i] and a[i+1]. Instructions that only apply to a[i] need to be carefully placed inside the loop in order for it to be vectorized. It's probably better to have the vectorizer handle such motion; nothing keeps these instructions from returning back to their original positions.

Sounds reasonable?

In general I wonder if this is really the best place to do this. It would be nice if the loop was canonicalised to be in this form given how cheap it is to do. Perhaps LoopSimplify? Not blocking this change, but something to think about.

New patch, rebased on latest ToT and using the different API implemented in the previous patch in D30086.

Thanks, I'll make that change and commit.

Addressed review comments, rewritten the pass a bit to be somewhat neater. D30086 is now committed now so this is ready to go if it looks ok.

Thanks. I'll make the last few changes requested and commit.

Splat is a synonym for broadcast as well, probably worth adding a mention.

In D30086#748613, @rengolin wrote:

In D30086#748611, @aemerson wrote:

When people add new intrinsics they don't really add tests to check the IR since you get assertion failures during the FunctionType creation if something goes wrong.

But without IR functions that actually uses them, there's no way to get the assertion, right?

If you have a look at prior art for adding intrinsics you'll see that actual verifier tests are only done for illegal combinations of constant value parameters. There are no illegal constant parameter combinations with this patch. E.g. Dan Berlin's r294341 doesn't come with a test, likewise with others.

In D30086#748597, @rengolin wrote:

Right, this is looking much better. Now, what about tests?

We'd probably need a bunch of tests to make sure that the intrinsics are accepted in the syntax that they're documented and rejected otherwise.

I'm not sure how's the best way forward, but probably just having IR in the right/wrong format and passing -validate or something expecting it to pass/fail would be a start.

cheers,
--renato

Ping. Ok to go?

Renato and I discussed this offline for a bit because we got our wires crossed a bit before. We agreed to simplify this code a bit more by extending createSimpleTargetReduction() to handle min/max by passing it the ReductionFlags. This essentially moves code from createTargetReduction() making it now just unwrap information from a RecurrenceDescriptor. Some other API changes done as a result.

Ok, so I've restructured the two functions a bit so that the simple (non minmax) reductions are generated from the createSimpleTargetReduction() function and the recurrence descriptor uses that for the simple cases, passing in the opcode.

• Split out SDNode changes into D32527 which is now committed.
• Added comments to the ISDNodes definitions.

Seems we haven't seen Justin active for a few weeks. @spatel are you ok for this to go in?

Rebased and updated with requested changes. Flags are now in SDNode, with an additional "defined" state bit to preserve semantics when intersecting flags.

In D32527#738046, @spatel wrote:

In D32527#738020, @aemerson wrote:

In D32527#738013, @spatel wrote:

Is there an advantage to this vs. just putting the flags in the base SDNode class? We're going to eventually want to use the flags for more than unary and binary nodes (eg, FMA), so I'd prefer to just go directly to that step.

I'm not the original author of the code but I think this is due to the extra 2 bytes of storage needed for the flags in each SDNode. With the current solution we only incur this cost if we have flags to store.

It's been a long time since D8900, but IIRC, the size argument was actually moot because we have "free" bytes based on the struct alignment (at least on common 64-bit systems). As mentioned there, the fact that the flags were only on binops was a limitation that we wanted to lift even back then, but it just hasn't been done yet. If you can do that in this patch, it would be great. :)

In D32527#738013, @spatel wrote:

Is there an advantage to this vs. just putting the flags in the base SDNode class? We're going to eventually want to use the flags for more than unary and binary nodes (eg, FMA), so I'd prefer to just go directly to that step.

Added more patch context.

Done.

No changes are needed on the MC side. The same target-specific reduction DAG nodes (e.g. AArch64ISD::UADDV) should be created and from then on everything should work as before.

At the moment nothing is emitting strict float reductions as no target supports it. We have it implemented for SVE but the IR type and vectorizer changes aren't upstream yet. The reason I've had to include it in this patch is because we want to agree on an intrinsics spec first without changing it later when SVE support lands.

Addressed review comments. Renamed the simpler create function to createSimpleTargetReduction() and added comments to both header and definition.