Hi Daniel, sorry for the delay.

LGTM.

New patch now only zero-extends for stores, removing the use of getBooleanContents.

I'll re-do this patch to unconditionally zero-extend for returns and also fix up the G_STORE legalisation to always zero-extend too. Given we don't have any target hooks that give us the information we need, let's go with just matching SelectionDAG behaviour?

I got my branches mixed up, this patch is missing test updates. Will upload a new one soon.

Overall I'm in favour of this change, so LGTM.

Hi Craig,

Looks like this change caused http://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O0-g/1944/ as well as others.

In D46889#1104823, @aprantl wrote:

Thanks for jumping on this Amara — I just wanted to point out that we ususally don't revert lldb changes that only break the lldb-xcode bot if they pass on the lldb-cmake bot at the same time. When this happens it usually means that the lldb Xcode project must be updated and it's too much to ask from all open source contributors to get access to a machine running Xcode to do this. Instead one of the Apple LLDB developers usually goes in and updates the Xcode project for them.

• adrian

Hi Pavel,

This caused a failure in green dragon: http://green.lab.llvm.org/green/job/lldb-xcode/6644

In D40864#1100492, @dexonsmith wrote:

Did this eventually go in?

I think I've addressed most of the issues. I've tried to clean up the computeValueLLTs function a little bit, still perhaps not ideal. Dead code has been removed (I think an earlier revision of the patch required it for the tests to pass). Also added the tests you wrote, thanks.

Some review comments not addressed yet, will add a test requested in a later revision.

Thanks. -time-passes shows that IRTranslator is now using around 2MB. It's also taking most of the compile time vs other passes like your initial analysis showed, but we trade off the legalizer doing much less work.

I've made the changes to not generate copies for the insertvalue/extractvalue, but how did you measure the memory consumption of the individual passes? With the change it now produces 4k lines of assembly on your test case, vs 3.5k assembly. The MIR is much larger but that's because of the extra GEPs and G_CONSTANTs. Compile time is now comparable to without this change.

In D46018#1095200, @rtereshin wrote:

Also, I looked briefly into https://bugs.llvm.org/show_bug.cgi?id=37397 recently, and I tried to apply this patch and see what it would do. It generated considerably more code (final assembly) comparing to itself and FastISel both (I'd eyeball the difference as 3x) and didn't seem to fix the issue with initializing upper bits of a boolean value.

Generally, I feel concerned about the quality (runtime performance) of generated code with this approach.

My thoughts were that we'd take a compile time hit with this, although in the end it was mostly neutral, but the code size would regress as you saw. The idea was to have a pre-legalizer combiner (Aditya had a prototype demo of one to show how the combiner API would work). Either that, or we have a clean up phase at the end of IRTranslator where we eliminate G_INSERT and G_EXTRACT pairs, perhaps with some caching of potentially redundant sets of these instructions due to pack/unpack regs as translation is happening, and then a quick pass to eliminate them without searching the whole function.

The actual problem here seems to be superlinear performance with this cutoff value. The 5x I mentioned was inappropriate as that was over a whole compile and compared to an old version of LLVM, before the store merging after legalisation patch for AArch64 was landed in December. The actual problem in my test case, which is admittedly a very large one, is that with the original value of 8192, I get around 330s runtime for my test case, at Max=2048 its 189s, Max=1024 its 34s, Max=512 21s, Max=256 20s. So somewhere between 512 and 1024 we start to see this superlinear compile time.

In D45541#1091441, @rtereshin wrote:

Hi Amara,

I think we lost 0.5% on compile time on CTMark because of this. Is there any scope left here for performance improvements? With the addition of the combiner landing later, I expect a further CT hit so anything we can do here would be good.

Just a quick question: 0.5% loss on the entire compile time or only some part of the pipeline?

I think we lost 0.5% on compile time on CTMark because of this. Is there any scope left here for performance improvements? With the addition of the combiner landing later, I expect a further CT hit so anything we can do here would be good.

I ran the test suite and SPEC2000/2006 benchmarks and didn't see any significant change with this.

Thanks, LGTM.

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.