In D59615#1439153, @t.p.northover wrote:

Did you look into a scalar variant of the intrinsic call instead? These instructions have non-vector variants (e.g. sqadd s0, s0, s0), and that's actually why the intrinsics exist in the first place. It'd be a shame to always require this extra work.

This looks quite involved as the scalar intrinsics have illegal types etc, and at the moment I don't have a lot of time to spend on this, it was just intended as a fix for the unstable tests: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/6257

Re-wording and making it more explicit who this is for.

Simplify logic and don't try to upgrade if IR is invalid.

In D59657#1438384, @hfinkel wrote:

I don't understand what this is supposed to mean to who the audience of this statement is?

I've put up a langref change as a separate review: D59657

Minor test tweak.

In D59444#1433415, @aditya_nandakumar wrote:

In general I'm not too fond of building very target specific instructions involving subregs with the MachineIRBuilder (As these are only likely to be used in a few places in the selector). Specifically for this case of building a subreg, you can say

Builder.buildInstr(COPY, {Dst},{})
  .addReg(Src, 0, SubReg);

which is not too much larger than (and doesn't require making every SrcOp bigger).

Builder.buildInstr(COPY, {Dst}, {{Src, SubReg}});

While adding CSE support for COPYs with Subregs is great, I'm not sure how often it'll trigger and be useful (If it does occur then great). Also should we start adding support for building other kinds of target instructions - Adding immediate operands/target index nodes etc?
My approach for building non generic instructions have been with using the .addImm(..).addReg(...)... pattern - but if others strongly feel the need to add the ability to pass in all of the operands at once to the builder interfaces, and enabling CSE for them, then we can go ahead with this change.

In that case I'll revert r356304 keeping in mind that we should take care not to enable COPY CSE in the selector in future, as it won't be subreg safe.

In D59444#1431750, @arsenm wrote:

I would think we would want to avoid any subregister indexes until as late as possible. Does the verifier reject them for G_* instructions?

Committed in r356213

In D59356#1430032, @RKSimon wrote:

In D59356#1429865, @aemerson wrote:

In D59356#1429581, @sdesmalen wrote:

@nikic thanks for pointing me to that discussion! I clearly misread the LangRef for this change :)
I agree it makes more sense to change these intrinsics and to make its accumulator argument always relevant (regardless of what flags are set) and AutoUpgrading older IR. Given that I'm currently working on this, I'd be happy to move this forward with patches and a proposal/discussion on the mailing list to change the experimental reduction intrinsics. @aemerson you expressed an intention to work on it later this year, do you have any objection to me moving forward with this now?

If you're going down the auto-upgrade route then I suggest proposing that we promote these from experimental to first class intrinsics. That way you can auto-upgrade form one intrinsic to another without any risk of breaking older code (i.e. you can't just start using an accumulator arg that before could be unused and therefore undef).

I'm not sure I agree - we shouldn't drop the experimental state unless we're certain the intrinsic is not going to need to be further tweaked in the future. I still think not including an accumulator argument at all would be for the best.

Can you switch to using MachineIRBuilder before committing.

In D58419#1419010, @arsenm wrote:

I think this should be symmetric with G_BUILD_VECTOR_TRUNC, so either a separate opcode or re-merge BUILD_VECTOR_TRUNC with BUILD_VECTOR

In D59356#1429581, @sdesmalen wrote:

@nikic thanks for pointing me to that discussion! I clearly misread the LangRef for this change :)
I agree it makes more sense to change these intrinsics and to make its accumulator argument always relevant (regardless of what flags are set) and AutoUpgrading older IR. Given that I'm currently working on this, I'd be happy to move this forward with patches and a proposal/discussion on the mailing list to change the experimental reduction intrinsics. @aemerson you expressed an intention to work on it later this year, do you have any objection to me moving forward with this now?

LGTM.

LGTM.

LGTM, can you rename the test to something more specific like "fallback-ambiguous-addp-intrinsic.mir"

In D58320#1418477, @peter.smith wrote:

I've no objections to adding the command line as a Darwin only option. Implementation looks fine to me although I've not got any prior experience with Darwin.

Since we can't use the proposed alternatives of -nolibc and -nostdlib++ reviving this patch as an Apple specific flag.

LGTM, though eventually we might want to change scalarizations to more efficient expansion lowerings.

LGTM with small change.

What about expanding the reductions into shuffle vector sequences? If we add support for that, such that the resulting constructed SDAG would be the same as the IR expansion shuffle vector sequence, then we pave the way for a move to using the intrinsics for all targets as the canonical form. So what we'd do is:

1. Add the expansion to shuffle vector sequences (instead of a naive implementation)
2. Move targets to use the intrinsic representation unconditionally. This means we don't need the useReductionIntrinsic TTI took any more. Targets' TargetLowering would need to specify which reduction kinds to expand using the new SDAG expansion code.
3. ...and as a result we can kill the ExpandReductions pass and finally move these intrinsics from experimental to fully supported and preferred representations.

Explicitly check for valid element sizes or error out if not valid.

Please add a test for regbank select.

I see. LGTM then.

Why is it valid for instructions to reference a dead frame index?

Apart from the aforementioned refactorings which can be done later, using MachineIRBuilder would also simplify some the code.

Addressed review comments. Also add an extra legalization check for source vector types being the same as the dest, this broke a lit test I missed.