Bah, yeah, sorry. Copied the wrong number.

LGTM

LGTM too. Would be good if an expert reviewed the target-specific changes, but they seem safe enough either way.

In D71432#1808202, @sdesmalen wrote:

In D71432#1803402, @cameron.mcinally wrote:

I just checked out the PseudoOps and they're interesting. If we could make generic XXX_PSEUDO(..., %passthru) PseudoOps, that would be a good path forward. I don't foresee any problems adding an extra passthru operand to the existing patterns, but maybe I'm missing something. If you have any insight, it would be appreciated.

Awesome, thanks for checking this out.

I don't see many issues with adding the extra passthru operand, but it could do with a bit of prototyping; what we've done downstream was very specific to having the false lanes zero'd and that is actually unnecessarily restrictive (but we never really had to care for the generic case). The challenge would be in expanding the pseudo. For some pseudo for that has a reverse variant, e.g. Dst = fsub_pseudo(Pg, Op1, Op2, Passthru)

we would for example expand this as follows:

z0 = fsub_pseudo_S(p0, z1, z2, z3)
  <=>
sel z0.s, p0.s, z1.s, z3.s
fsub z0.s, p0/m, z2.s
 
z0 = fsub_pseudo_S(p0, z1, z2, z2)
  <=>
movprfx z0, z2
fsubr z2.s, p0/m, z1.s
 
or a special case for zero'd lanes:
 
z0 = fsub_pseudo_S(p0, z1, z2, <zero>)
movprfx z0, p0/z, z1
fsub z0.s, p0/m, z2.s

The indexed FMLA instruction has three input operands and this is a case where we can't relaxation the register constraints:

%dst = Pseudo_FMLA %a, %n, %m, %index
(to implement: %dst = FMLA %a + %n * %m[%index])

We cannot recover from a register allocation where %index is used as %dst, e.g.

Z0 = Pseudo_FMLA Z1, Z2, Z0, <index>

(there is no other variant we can use to recover from this, so those will need to retain a constraint that $Zd = $Zs1)

Ok, thanks for the clarifications. Looks good to me, but it would be good to have experts in OpenCL/Cuda/AMDGPU review the target specific changes.

In D71432#1788807, @sdesmalen wrote:

While we don't support the generic case in our downstream compiler, we do have special support for the cases where the false lanes are zeroed or undef. Using the predicated MOVPRFX instruction, the false lanes can be zeroed relatively cheaply using e.g.:

movprfx z0.s, p0/z, z1.s
fsub z0.s, p0/m, z2.s

This avoids having to emit an explicit sequence of a splat and select / predicated mov to zero the false lanes. We match the operation + select into a Pseudo instruction (e.g. FSUB_ZERO or FSUB_UNDEF), that is expanded after register allocation (in the AArch64ExpandPseudoInsts pass) into the appropriate instructions.

Even if we don't care about selecting a passthru value for the false lanes, there is still value in creating the Pseudo. The lack of a tied-operand constraint for the Pseudo gives the register allocator more freedom to come up with a better allocation. Combined with the commutative property of some instructions or by expanding to their reversed variants (like SUBR vs SUB), we can avoid a number of unnecessary register moves.

We've been thinking about some ideas on how to make this support more generic to allow supporting the general use-case of:

%Res = FSUB_PSEUDO(%Pred, %Op1, %Op2, %Passthru)

Depending on the value for %Passthru, this can be expanded to use a movprfx or in the worst case an explicit select.

Ideally we'd use a Pseudo for most operations so that we can use this as a generic mechanism that natively supports the passthru value and benefits from better register allocation.

A bit of prototyping would be required though, as our downstream compiler only covers a limited use-case. We've also had to deal with some corner-cases, but I'd need to refresh my memory on the details of those before I can comment on those. I'll try to dig up some more details!

This is looking pretty good to me, but I'm ignoring some of the target specific code that I'm not familiar with.

Add a class for the op+select patterns.

LGTM

Ah, ok. I see your point now.

In D71432#1782606, @efriedma wrote:

Adding patterns for vselect of various operations seems reasonable in general. The patterns are simple enough that it's not a big deal to repeat for a bunch of instructions.

Remove some leftover debugging junk...

In D62731#1779872, @mibintc wrote:

In D62731#1778597, @michele.scandale wrote:

I've noticed you removed the change for CompilerInvocation.cpp about the initialization of the codegen option NoTrappingMath. Was that an accident?

Thanks again Michele. I'd like to get rid of Opts.NoTrappingMath, but I haven't been bold enough yet.

Oh, or maybe I misunderstood. I assumed Andy meant a single intrinsics with an extra boolean argument, but now I'm not so sure...

In D69281#1759494, @andrew.w.kaylor wrote:

I would prefer a single intrinsic/opcode with additional predicates to handle signalling vs. quiet. My main reason is consistency with how the IEEE-754 spec describes comparisons, but that's a weak argument. @craig.topper mentioned to me that there are some additional opcodes that might need to be duplicated if you treat signalling and quiet as separate operations.

LGTM

LGTM

Attn: @craig.topper @eli.friedman. The FMF behavior looks like it will take a performance hit with the switch to default IEEE conformance. Is this something that needs to be publicized on llvm-dev before merging? Users who don't care will want to add an explicit denormal-fp-math attribute to their code.

Ah, I didn't realize there were test differences from this change. It should probably be combined with D69988...

LGTM

LGTM

Adding @spatel and @eli.friedman. My confidence level is low for this review...

Oh, it's gone now. Maybe it was a temporary hiccup. Will review this afternoon...

Hmm, something is weird (with my Diff, at least). It looks like changes are listed twice (e.g. lib/IR/Verifier.cpp and llvm/lib/IR/Verifier.cpp). Maybe this is an SVN->GIT side-effect and the patch needs a rebase?

LGTM. Will give other reviewers some time to review though...

Is isFMAFasterThanFMulAndFAdd's MachineFunction argument currently used? Or is that coming in a follow-up Diff? I might have missed it...

LGTM

LGTM

In D57504#1725445, @rengolin wrote:

LLVM is a very fast moving target, stopping the world to get the IR "right" doesn't work.

A good example to look for is the scalable vector IR changes that have gone through multiple attempts and are going on for many years and still not complete...

These things take time, rushing it usually backfires. :)

This looks good. Is it possible to write tests for the AMDGPU specific change?

In D69275#1725323, @kpn wrote:

In D69275#1724146, @cameron.mcinally wrote:

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.

For a first cut I was hoping to avoid dealing with Custom lowerings. That's why there's no v4* test cases. Is this something that can be handled when the X86 backend support gets to this point?

In D57504#1724902, @rengolin wrote:

In D57504#1724217, @cameron.mcinally wrote:

Code explosion is the symptom, not the sickness. It's caused by using experimental intrinsics. Experimental intrinsics are a detriment to progress. They end up creating a ton more work and are designed to be inevitably replaced.

I think this is a big hammer argument for a nuanced topic.

In D57504#1723586, @simoll wrote:

DevMtg Summary

• There will be a separate RFC for the generalized pattern rewriting logic in LLVM-VP (see PatternMatch.h). We do this because it is useful for other efforts as well, eg to make the existing pattern rewrites in InstSimplify/Combine, DAGCombiner work also for constrained fp (@uweigand ) and complex arithmetic (@greened) . This may actually speedup things since we can pursue VP and generalized pattern match in parallel.

In D57504#1723466, @simoll wrote:

In D57504#1720792, @shawnl wrote:

Are predicated vector instructions not just a special case of DemandedBits? Why can't we leave out the .vp. intrinsics, and just generate the predicate with DemandedBits? That way you do a predicated vector operation like so (in zig): As the example makes clear, this optimization would have to be guaranteed in order for the generated code to be correct (as the predicate avoids a divide-by-zero error).

var notzero = v != 0;
if (std.vector.any(notzero)) {

v = std.vector.select(5 / v, v, notzero);

}

What you describe is a workaround but not a solution for predicated SIMD in LLVM.
This approach may seem natural considering SIMD ISAs, such as x86 SSE, ARM NEON, that do not have predication.
It is however a bad fit for SIMD instruction sets that do support predicated SIMD (AVX512, ARM SVE, RISC-V V, NEC SX-Aurora).

As it turns out, it is more robust to have predicated instructions right in LLVM IR and convert them to the instruction+select pattern for SSE and friends than going the other way round.
This is what LLVM-VP proposes.

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

I did not actually require any TableGen changes, but was able to represent the overloaded intrinsic types using existing mechanisms. This solution looks correct to me, but if I'm missing anything here, please let me know ...

Good catch. Thanks. LGTM.

Both the ASan build:

Fix incoming. Sorry for not running the ASan tests...

Apologies, @pcc. Looking now. Will revert if it's not obvious...

Recommitted this patch with Ocaml test fix. I was not able to find quick-start documentation for the Ocaml bindings, so the Ocaml failure has not been tested. That said, the failure mode seems extremely low risk. Will monitor the buildbots for problems...

@gribozavr I see that you also reverted @RKSimon's commit for the OCaml/core.ml failure:

In D61675#1703749, @cameron.mcinally wrote:

I reverted it in r374354. Please test before re-landing.

Hmm, how do I run those tests? I did not see that failure with check-all.

That's a pretty straightforward failure. It's just a one-line IR change:

fsub float {{.*}}0{{.*}}, %F1 -> fneg float %F1

I reverted it in r374354. Please test before re-landing.

This looks good to me, but other users should review it as well...

Sorry for the slow response. I've been busy with other projects.

Looks pretty good. This could probably use some tests in TypeAndConstantValueParsing of llvm/unittests/AsmParser/AsmParserTest.cpp. In particular, an isa<VScale>(...) test would be good. And maybe a negative test to make sure something like <undef x 2 x f64> can't be parsed.

What happens if we see IR like:

LGTM with a moderate confidence level. Maybe give other reviewers a day or so to comment before committing.

LGTM

Would it be better to do an attributes #0 = { ...} style change? E.g.:

For posterity's sake, @andrew.w.kaylor just suggested adding a nftz fast math flag:

Some data points...

Address review by @lenary.

In D61675#1671688, @cameron.mcinally wrote:

Someone could argue, though, to just use a constrained strict FSub if you care about DAZ/FTZ. That seems like a valid solution. But, that is really treating DAZ/FTZ like a rounding-mode, not underflow. It would be a heavy hammer to enforce all the side-effect concerns when the user really only cares about DAZ/FTZ. In other words, we'll be losing significant performance in an attempt to gain significant performance.

In D61675#1671602, @spatel wrote:

And I think the case where a user changes MXCSR bits is UB for C/C++ ( https://bugs.llvm.org/show_bug.cgi?id=8100#c15 ). So x86 never has a problem in theory, but it might in practice because users believe that twiddling MXCSR bits is allowed?

My confidence is somewhat low though...

In D61675#1670603, @spatel wrote:

On 2nd thought, that doesn't really make sense for CSE. The real question is whether we should canonicalize the binary fneg to unary fneg in instcombine. And should that happen before/after/concurrent with this change to clang?

Maybe a dumb question, but what happens if two incoming PHI values have different FMFs set? Does the PHI's FMF override those? If so, is that safe?