Sorry, I've not had much review time this week but I'll take a proper look tomorrow or Friday.

To prevent some duplication of event I just wanted to update my previous comment and say all the dependent patches have now landed and I'm planning to add _u intrinsics/builtins for the integer MLA instructions soon.

Ping now the work to disallow vscale constant expressions is complete (D145404).

This option was our pragmatic way to ensure scalable vector based toolchains remained useful whilst the kinks were worked out. We've a few releases under our belts now and I feel that enough works to consider further errors as something we should not be hiding.

We knew when landing the original patch that it'll likely bite us, but at that time there just wasn't a good enough reason to warrant the loss in code quality.

ping

@cameron.mcinally : Given D141924 can this patch be abandoned?

Restored simplification for case where isSupportedGetElementPtr() is false.

A few recommended improvements but otherwise looks good.

Tighten up a couple of function signatures.

I suspect we'll want to replace replace-intrinsics-with-veclib-sqrt.ll with more exhaustive testing for all the intrinsics but that's a job for another day. I've a couple of observations but otherwise the patch looks good.

Fair enough. Sorry for the misunderstanding.

In D142998#4170452, @sushgokh wrote:

@paulwalker-arm Any comments/suggestions on this ?

Rebase after updating and precommiting the tests.

Add matching combine for "A | (~A & B) --> A | B".

Thanks for the education @nikic. I'll implement that transformation as well.

In D145157#4164759, @nikic wrote:

Missing scalar test coverage. Please also add relevant proofs to patch description (e.g. https://alive2.llvm.org/ce/z/oKiodu). What about the de Morgan conjugate?

Add scalar test cases and fixed switch operand test.

This is a partial fix for https://github.com/llvm/llvm-project/issues/58690

Updated to include correct handling of poison.

Need to be more strict with the handling of poison.

ping

Not a strongly held view but is there value in restricting the combine to pre-operation legalisation? I ask because I wonder if they'll be a point during legalisation where somebody specifically wants the ISD::VSCALE node. Perhaps a different/better option is to move the logic into SelectionDAG::getVScale(), which can take a default false bool to disable the optimisation?

A couple of minor issues but otherwise looks good.

This patch follows on from a conversation on D144624. I've tried to keep things conservative (i.e. no llvm.vscale() detection or use of vscale_range attributes) so as not to change existing behaviour, hence no test changes.

One minor suggestion but otherwise looks good.

Thanks for the information @nikic . I'm assuming the road to ptradd will not be short? so I'll investigate adding something to SCEV so we can move towards updating the LangRef for vscale.

I realise this patch is partly refactoring but it does seem to increase the value of scalable vector GEP ConstantExprs, which is inconsistent to your view on D134648? Is it possible for SCEV to have its own representation of vscale that is independent to the Constant class hierarchy?

There's likely other combines that will also need to be updated but this is the most important one blocking D143767. I'd like to take a more holistic look at the others as part of work to unify some code paths. For example, I want to canonicalise sve intrinsic calls where the predicate is all active to use the "undef" intrinsics so that some code duplication within the code generator can be removed. With that said, please shout if you know of something critical that should be handled before D143767 lands.

In D143767#4138049, @dewen wrote:

Hi, thank you. When will this patch be pushed to the master?@paulwalker-arm