LGTM!

• I realised that instead of calling getValue() and asserting the cost is valid I can just change a few functions to return InstructionCost instead. The code simply compares the benefit and cost and decides to outline based upon that.

LGTM! The canonicalisation looks sensible to me.

LGTM! Thanks for adding the test.

LGTM! Seems like a sensible rename to me.

Hi all, this is just a thought and I hope I'm not confusing things further (!), but we could also have something like:

• Following discussions on D94069, I've removed unnecessary checks for isValid when comparing costs.

LGTM! I'd blame the reviewer for the bug in the previous patch. :)

Hi folks, just a gentle ping to see if anyone has chance to have a quick look?

• Updated documentation as per review comments.
• Fixed an issue with using value->prettyPrint on a null ptr.
• Reworked the code that sets vectorize.enable.

LGTM! Thanks for making the changes.

LGTM! We already have precedent for this with the nxv4f16 type anyway.

Hi everyone, I realise that most people have probably been on holiday recently, but just a gentle ping here to see if anyone could take another look? Thanks!

Hi @CarolineConcatto, I wasn't sure whether I needed to change computePredInstDiscount because I've assumed (possibly wrongly!) that we shouldn't be encountering invalid discounts (costs). At the end of computePredInstDiscount it calls *Discount.getValue(), which will assert if the cost is invalid so there is some protection. When we add scalable vector support I doubt we can even call this function in it's current form because we know the scalarisation overhead will always be invalid.

LGTM!

• Added support for using SVE with fixed length vectors.

Hi @paulwalker-arm, I can add support for fixed length vectors in this patch. It's no trouble really. Aren't you supposed to be on holiday btw? 😉 Bored of The Witcher?

• Rebase

LGTM!

• Removed assert in Instruction::ExtractValue case in getInstructionCost.
• Rebase.

LGTM. I agree the FIXME in the code isn't ideal, but I understand why it's there and is only a temporary measure until we can fix the loop to support scalable vectors.

LGTM! Hi @tschuett I agree I think it's better to leave any possible refinement of getMaxVScale to a later patch as it may require new flags.

After some recent upstream discussions in the SVE sync call this patch is no longer needed.

LGTM!

LGTM!

Happy with latest version. Looks good to go!

LGTM!

Hi everyone, thanks for all the review comments! Unless anyone has strong objections I'd like to land this patch on Thursday on Sander's behalf, since he is on holiday at the moment.

LGTM with the nits addressed!

LGTM!

LGTM!

Hi @kmclaughlin, I've just got one function left to review, but your patch looks good so far with lots of thorough testing! I just had a few minor comments.

• Fixed sorting issues with InstructionCost header + class declaration.
• Added assert that ExtractValue cost is valid in LoopVectorize.cpp.
• Added additional tests to InstructionCostTests.cpp

HI @paulwalker-arm, right that makes sense and thanks for the explanation!

If you can find a test that exposes the need for this split function with generic IR that would be really helpful I think - LLVM has managed for a long time without needing this split function so something has changed. I suspect it's probably just the new intrinsic that now exposes this code path.

• Added increment and decrement operator tests.

• Fixed typo in InstructionCost.h - replaced LLVM_ANALYSIS_INSTRUCTIONCOST_H with LLVM_SUPPORT_INSTRUCTIONCOST_H