In D45543#1074883, @dsanders wrote:

In D45543#1066099, @aditya_nandakumar wrote:

Maybe we could have all the tests in one file called prelegalize-combine-extloads.mir

I went with three files to match how we've been organizing the tests for the other passes but you raise a good point here. There's a good argument for the combiner being tested with one file per CombinerHelper::try*() function. I think that's probably a better organization.

In D45543#1071617, @aemerson wrote:

I'm assuming this is in Target/AArch64 because other targets haven't been updated to use the new opcodes yet? We do eventually want to use these representations for every target though right?

That's right. As you say, we'll want to support it in every target that supports the extending loads (which is most if not all of the in-tree targets). However, until the new opcodes are legal for those targets, there's not much point in combining them only to revert them back to load+extend in the legalizer.

One other thing to mention is that we don't have a target-independent combiner in GlobalISel at the moment. Each target implements its own combiner(s) and makes use of code in CombinerHelper (where appropriate) to share code. I expect these combines to be used by multiple targets so I've put the bulk of the code in CombinerHelper but each target will need to add a pass and call to it.

I'm assuming this is in Target/AArch64 because other targets haven't been updated to use the new opcodes yet? We do eventually want to use these representations for every target though right?

LGTM, with addition of -verify-machineinstrs to the tests.

LGTM.

The motivating use case for this was actually fixed in clang, r325478. Although this may be useful in future for other front-ends/clients of LLVM, this can be revived if needed later.

LGTM, thanks.

In D45336#1060178, @RKSimon wrote:

In D45336#1060080, @aemerson wrote:

The other issue is that while these intrinsics were experimental (it was on my todo list for later this year to change that), AArch64 has been using them with their original intended semantics for a while now. If we change that, IR generated from a released compiler will be miscompiled since it now becomes legal to fold away undef accumulator reductions to undef, unless we do some IR auto-upgrading based on the bitcode version.

If they are flagged as experimental surely we can't be held resposible for changes in behaviour?

AArch64 was the test guinea pig for this new representation, and it's proved to be a smooth transition. If you change the semantics now, even if the intrinsics are still experimental in name, IR from LLVM 6.0 may be silently miscompiled if someone implements a valid optimization based on your new proposal. That's a fact, and I don't think this patch review is the right place to discuss that if you want to do this, I suggest you send a new RFC or revive the old one.

Simon asked this on the PR, let's continue the discussion in one place:

Do we really want to completely ignore an intrinsic argument depending on the fast flags? There might be valid reasons to want to include an accumulation value.

The raison d'être for the argument is for ordered reductions, and the intrinsics were designed to allow the expression of the reduction idiom only, in light of newer vector architectures where the previous representation was inadequate. The use of an accumulator argument for fast reductions wasn't necessary, so the semantics were supposed to be defined in the most minimal form. The accumulator can easily be expressed as a extractelement->op->insertelement sequence. The question here I think becomes our good old friend: what should the canonical form be?

In D45336#1059781, @gnzlbg wrote:

@aemerson

I think I missed out a detail when I wrote the langref, original motivation of the scalar accumulator argument was for the use in strictly ordered FP reductions only. I.e. when the intrinsic call has no FMF flags attached then the accumulator argument is used, otherwise if there are no FMF flags then the argument is meant to be ignored.

Why do we need the accumulator for this case? That is, why can't we just do:

result = vector[0];
for i in [1, vector.len) {
    result = binary_op(result, vector[i]);
}
return result;

---

I also wonder whether requiring fast-math to allow tree reductions is overkill. Tree reductions can be implemented reasonably efficiently in many architectures, while linearly ordered reduction appear to me to be more of a niche. Therefore, I wonder if it wouldn't make more sense to add llvm.experimental.vector.reduce.tree.{add,mul} that perform tree reductions without requiring fast math, and to just call those from here if fast-math is enabled.

Hi Roman,

LGTM.

In D42512#1026016, @rnk wrote:

This doesn't fix the crash, though. We just assert later now.

LGTM, can you also make the test name a little more specific, e.g. 'vecreduce-propagate-sd-flags.ll'?

Please add a test.

In D43108#1023300, @nruslan wrote:

By default, stack probes are enabled (i.e., -mstack-arg-probe is the default behavior) and have the size of 4K in x86.

This part what I wanted to clarify, -mstack-probe-arg is enabling stack probes if the ABI requires it only, not for other reasons like security.

Can we clarify the meaning of this option a bit. The doc you've added here is saying that -mno-stack-arg-probe disables stack probes. Then what does -mstack-arg-probe mean specifically? Does it mean that only stack probes for ABI required reasons are enabled, or probes are done even in cases where the ABI doesn't require them? Either way, the doc needs to be clearer on the exact purpose.

Seems reasonable.

Changed to use errorUnsupported() so we get some diagnostics first.

r325550.

As stated in PR36345 I'm committing this now to get it into 6.0.

Thanks, I'll commit this with those changes and put up another patch shortly for the copies in the other direction.

Thanks for refactoring it. LGTM with a minor comment addition.

Sorry I didn't see this, I need to fix my email filters.

@hans if you're happy with this could you commit to the branch?

@hans if you're happy with this could you commit to the branch?

Thanks, I'll commit this first and follow up on that. At first glance they're using different slightly different register classes so I'm not sure how correct it is to use that.

Ping.

Simplified the logic a bit and added an example in the function documentation of complex and simple addresses.

In D42612#992814, @Gerolf wrote:

This is very close now. Could you add an explicit examples (eg show the IR) showing which initialization remain slow (Complex) and which are fast now? This should also address the spirit of Adrian's question I think.

Thanks
Gerolf

Addressed feedback.

New fix, I think the issue was that IMPLICIT_DEFs weren't constraining their dest register, while COPY selection didn't constrain the source.

Looks fine.

Meant to say, I'm *not* familiar with this part of the codebase.

In D42612#991476, @Gerolf wrote:

Thank you drilling into this! I have a few questions below. Also, could you comment on the time savings you measured for your implementation?

-Gerolf

With this change, the test case I was using completed compiling in about 45 seconds, a significant portion of which was spent in the front-end/elsewhere in the compiler.

Updated with some more comments.

LGTM from a GISel point of view, I can't say I understand Windows enough to say the tests are right.

Right, it's not clear to me how we'd avoid crashing later on since in this case it happens in a later pass, not in isel. If report_fatal_error does indeed always triggers crash diagnostics in clang then this is no better than assertion failures. Perhaps emitting the diagnostic first with errorUnsupported might be better than nothing.

LGTM with some minor comment fixes.

I think this is ok to progress now. We're happy with the overall design, although the pre-legalize pass is probably unnecessary at this moment, so leave that for another patch?

For GlobalISel we haven't had to have very target specific code in the IRTranslator for this before. @qcolombet any opinion on whether we want to include target/ABI specific code into the IRTranslator? My feeling is that we want to avoid this, the alternative I can see is to introduce a new opcode for G_DYNALLOCA and lower that at isel.

I've added two kinds of warnings, one for targets which have incomplete GISel support, and another for unsupported optimisation levels (for ARM64 -O{1,2,3,s,z}).

@kristof.beyls @rogfer01 Could someone from ARM do a quick test of this? I don't have an ELF target handy to test it myself and I'd like to get this fixed in the release branch.