No problems, feel free to close, thanks!

In D57953#2289359, @masakiarai wrote:

I implemented this as a new optimization pass, so I will upload it.
I'm thinking of getting a new Phabricator review number and linking it from this comment section.
Is this a reasonable procedure?

In D87337#2272515, @myhsu wrote:

Ping. Just want to check out if we all agree to flush out this patch series first before supporting optsize / minsize as per our discussions in the RFC thread

Implementation-wise, this looks good, but I wonder if we could join minsize with this using the same (or very similar) controls.

I'm a bit wary of all the new asserts. The vectoriser is reasonable self-contained, and if you check for the scalable flag at entry points, you can assume safety inside.

I think this is a good direction, too.

In D79317#2023267, @mehdi_amini wrote:

FYI we document now to not use "Patch by" but to use git commit --amend --author="John Doe <jdoe@llvm.org> to have the proper commit author recorded, see https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Ok, turns out the failure was a flaky test due to parallelism (CMake deps not right?), but the build finishes and the tests all pass.

@rriddle, do you know what's failing in the test build?

+1 to Chris, Mehdi and John comments. I think I get the idea, but the text is certainly not conveying that, for me.

Oh, already merged, ignore me. :)

In D70456#1959386, @fhahn wrote:

Yes, the lowering as done in this patch could also have been done exclusively by the frontend without functional difference.

Hi Florian, cool work, thanks!

Based on mailing list discussions I think it should be fine for the AVR backend to be built by default. The code owners should prepare for a higher influx of bugs and be extra-responsive for a while. Thanks!

Parachuting here, but, for reference, this is the builder that seems to be mostly green for a while: http://lab.llvm.org:8014/builders/llvm-avr-linux

Hi Evgeniy,

In D72002#1806077, @arsenm wrote:

The 3 big things SelectionDAG uses for this are in the same category. read_register, write_register and inline asm. The rest doesn't really apply. The alternative is every target needs to implement the exact same string parsing which is unneeded duplication

Hi Matt,

Some inline comments, but otherwise, LGTM. Thanks!

LGTM, thanks!

Hi Sander,

Hi David,

This change may conflict with your other two. How are we supposed to review them? Are they a set?

In D71250#1778881, @ebrevnov wrote:

These extra branches are very short leaved and optimized out by SimplifyCFG which happens just 3 passes after the vectorizer.

This change introduces a number of new branches in the code. While not necessarily the hottest code, it may prove significant on short loops, especially nested loops. Without evidence in the form of benchmarks etc. it's hard to justify.

Thanks Francesco, looks ok to me, but I'll leave to @jdoerfert or @sdesmalen to approve.

In D71053#1772108, @ebrevnov wrote:

Yes, this case came from the real world benchmark. This change gives +15% on it.

Is this change inspired by a real world case? If so, how relevant / pervasive is this case?

As far as I can see, this shouldn't introduce behavioural changes and does add some nice cleanups.

In D68577#1730827, @Ayal wrote:

None from me.
@rengolin , @fhahn - ok to land?

In D57504#1725429, @cameron.mcinally wrote:

But it just seems silly to me to implement something twice: Occam's razor. We'll have to work the kinks out somewhere -- so why not push directly to the goal...

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.

In D69103#1724045, @simoll wrote:

I understand. We will repackage the github code into digestible commits and put them on phab. I'll get back to you when the patch sets leading up to full scalar codegen are ready.

In D69103#1723824, @simoll wrote:

Great! How do we mark the target to make it clear it's experimental?

This plan has all the requirements for adding a new target (http://llvm.org/docs/DeveloperPolicy.html#new-targets), mainly: ISA document, current implementation, code owner, buildbots. You just need to make sure it's marked as *experimental*, as stated in the policy.

So, IIUC, this is changing tryCreateRecipe to move the interleave recipe creation to the caller, buildVPlanWithVPRecipes. The dependencies with the sink values is recorded initially, then the plans are created, then the sinks are applied and, if any, the interleave groups.

This work was mentioned on the SVE discussion about predication, adding arm folks, just in case.

In D67602#1684353, @arsenm wrote:

For this one: why only AMDGPU tests? Would be good to have at least ARM32/64, since they should be working with GlobalIsel, no?

One target is sufficient for new feature additions

In D67601#1671349, @jyknight wrote:

I think this function probably ought to be renamed. It's really not clear from its name that it's intended for such a narrow purpose -- _only_ for implementing the read_register/write_register intrinsics.

Added some context comments in D67601.

For context, read_register was introduced to allow use of the "global named register" GNU extension.

Nope, Asan failures from the sanitizer commit.

This is a tricky one which may vary depending on the libraries available on different systems. Which toolchain is this? Can you add more context?

I see, makes sense. LGTM, thanks!

I understand (and agree with) the reasoning of this patch, but wouldn't this also make it harder to test the current behaviour?

Right, ArchExtKind was supposed to be shared between Arm and AArch64, but that ship has sailed a long time ago. LGTM, thanks!

In D65197#1599102, @Meinersbur wrote:

Before we had the monoropo reviewers frequently asked to split patches into the LLVM and Clang part. With the monorepo, I am not sure the rule still needs to be followed. At least, I did not expect LLVM documentation in a clang patch, so sorry for the non-applicable comment.

In D53137#1597022, @rovka wrote:

I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.

Better this way, let's keep the changes contained to one per patch. If there's interest to change that behaviour or not, it should not hold this patch, which looks good to me now. Thanks!

In D62997#1591113, @psamolysov wrote:

@hsaito The test no_switch.ll failed because remarks aren't written now by default, so without -pass-remarks-analysis. I've just removed the corresponding check from the test. But, I think, the NFC label from diff/commit should also be removed in this case because the behavior is changed a little bit. No other problems with tests in the LoopVectorize directory. If this diff is applied, the entire set of tests passes.

As far as I can see this is an NFC refactoring with obvious benefits to readability and extensibility. LGTM too. :)

In D53613#1566137, @lebedev.ri wrote:

It's best to submit a new review.
Else no one sees the description as a mail.

Adding llvm-commits for wider audience

lgtm, thanks!

In D62997#1551946, @psamolysov wrote:

The diff has been updated: private methods were removed, the function 'reportVectorizationFailure' was moved to LoopVectorize.cpp and declared in LoopVectorize.h (namespace llvm). I've removed the passName parameter and pass LV_NAME as pass name to ORE but this change breaks 3 regression tests:

I agree with Hideki, this could be in LoopVectorize.h.

Sorry Pavel, tough week. it's at the top of my list.

@huntergr do you have an account on bugzilla? I couldn't CC you on that bug.

Committed as r362736

Much cleaner, thanks! I think we should deal with extending the functionality in a separate patch, since that's already a good localized improvement.

In D62478#1518679, @psamolysov wrote:

I don't understand why having a function with a lot of arguments is better than a private method getting access to the class' members. Why are private methods in C++ at all, they also are used only from class methods and can be replaced with static util functions getting required class members as arguments.

In D62478#1518814, @psamolysov wrote:

I believe the LoopVectorize class also should use this function since there are a number of occurrences of the same pattern

In D62478#1518099, @psamolysov wrote:

@rengolin It is because the function uses OptimizationRemarkEmitter *ORE, a member of the LoopVectorizationLegality class.

Looks nice, thanks! Though, does it need to be a member function? Why not just a local static function?

In D62110#1511129, @nickdesaulniers wrote:

LoopVectorizationCostModel::expectedCost default initializes its return value, an instance of LoopVectorizationCostModel::VectorizationCostTy declared as:

using VectorizationCostTy = std::pair<unsigned, bool>;

so looks like it's possible.

Can expectedCost ever return zero?

looks obviously good to me. thanks!

In D32530#1477041, @hfinkel wrote:

Exactly. Non-constant values can become constant. Constant values can be guarded by vscale-dependent runtime guards (both hand-written and compiler generated). My preference is to leave this not restricted to vscale == 1 values, but rather allow all values that can be supported at runtime, and have it be UB if, at runtime, the relevant index is not available.

In D32530#1466746, @huntergr wrote:

So if we wanted to keep them as intrinsics for now, I think we have one of three options:

1. Leave discussion on more complicated shuffles until later, and only use scalable autovectorization on loops which don't need anything more than splats.

In D32530#1464722, @hsaito wrote:

If we are going for more than element-wise Instructions, we need to have well defined and agreed semantics for each of those, and that should be part of the LangRef for each such Instruction.

In D32530#1456113, @huntergr wrote:

• Clarified that the runtime multiple is constant across all scalable vector types, even if the constant value isn't known at compile time.
• Removed extra whitespace.