Thanks @efriedma for working on this. The overall approach seems good to me!
Mostly added some nits, with the exception of one question on what to do with a shufflevector with fixed-width mask and scalable source vectors.

Superseded by D68203.

Thanks for the updates to the patch @dancgr , LGTM!

In D71215#1832298, @efriedma wrote:

LGTM with one minor suggestion.

Sorry about the delay; I didn't understand the dependency relationship between this and D72758.

No worries, thanks for reviewing!

The Download raw diff button seems to run into a Unhandled Exception page.
arc patch doesn't work either because the patches are missing the llvm/ and clang/ prefix to the file paths.

The Download raw diff button seems to run into a Unhandled Exception page.
arc patch doesn't work either because the patches are missing the llvm/ and clang/ prefix to the file paths.

Gentle ping (I think I've addressed all comments on the patch)

In D68203#1820191, @efriedma wrote:

LGTM.

I was about to commit this patch today when I realised the differential wasn't yet in accepted state. Any objections for me to land the patch?

In D72799#1829698, @dancgr wrote:

I'm not sure on some parts, but I have prepared a major update for this patch that I hope will fix most of @sdesmalen concerns.

Great, thanks for updating your patch! The patch is nearly good to go I think, just one more little change needed to remove _m.

LGTM [with the caveat that we need to revisit the modelling of the FFR register and get rid fo the PseudoInstExpansion at a later point, as discussed during the previous sync-up call]

LGTM!

LGTM!

Thanks for efforts on this patch @dancgr!

Thanks for these fixes. LGTM!

The fix seems sensible to me.

In D71216#1817936, @efriedma wrote:

Please add a comment explaining each isScalableVector(), to make it clear why we're excluding them. Otherwise LGTM

Thanks, I'll add some comments for these cases (and do this for future cases I find as well).

Changed scale argument to VSCALE from int64_t to APInt.

added context to the patch

• Added two fixes to DAGCombine where we need to bail out if the type is scalable (the tests ran into some assertion failures otherwise).

• Code in SelectAddrModeFrameIndexSVE now checks if index is a FrameIndexSDNode (rather than assume it is one).
• Fixed whitespace and updated comment.

• Simplified the llvm.vscale intrinsic to no longer take the scaling argument. This means the scaling is left to an explicit mul which has explicit wrapping flags. This in turn simplifies the code in SelectionDAG for merging the multiplier into the VSCALE ISD node (this should only happen when it is known that no wrapping occurs).
• Also made llvm.vscale overloadable, so that it can return vscale in different types.
• Added special patterns for mul vscale, 1 and mul vscale, -1.

In D68203#1809517, @lattner wrote:

I'm not a comptent reviewer on the details for this whole patch, but the general intrinsic approach LGTM!

Awesome, thanks for having a look!

• Changed some mul into shl in the test file, to test fold into vscale isd node.
• Addressed comments.

In D71432#1803402, @cameron.mcinally wrote:

I just checked out the PseudoOps and they're interesting. If we could make generic XXX_PSEUDO(..., %passthru) PseudoOps, that would be a good path forward. I don't foresee any problems adding an extra passthru operand to the existing patterns, but maybe I'm missing something. If you have any insight, it would be appreciated.

Awesome, thanks for checking this out.

In D71636#1807522, @simoll wrote:

define i32 @vscale() {
  ret i32 ptrtoint (<vscale x 1 x i8>* getelementptr (<vscale x 1 x
i8>, <vscale x 1 x i8>* null, i64 1) to i32)
}

Whether this returns vscale or something else depends on the ptrtoint mapping of the default address space.. and <vscale x 1 x i8>* null might not be i32 0 for some targets. You might get away with subtracting the integer value of null ;-) (.. i guess @arsenm is more familiar with the topic of address spaces and what may be assumed about them given his recent DevMtg talk).

My understanding was that for the default address space, nullptr always has the bit value 0. I don't really understand this topic enough to know the intricacies around the ptrtoint mapping of the default address space, but if this is used for sizeof, I don't see why it couldn't be used for vscale (taking into account the check for larger alignment).

Added LLVM IR support to this patch; as intrinsic and constexpr pattern (following suggestions in D71636)

Hi Renato,

LGTM as well!

Thanks for adding the extra constant fold @huihuiz!

Hi @cameron.mcinally, thanks for sharing this patch!

In D71556#1786465, @efriedma wrote:

I'm not sure it's legal to transform a non-faulting load to a load with a non-faulting flag? At least, we'd need to consider the implications of that very carefully. In particular, I'm concerned about the interaction with intrinsics that read/write FFR. I mean, you could specify that loads marked MONonFaulting actually write to the FFR register, but that seems confusing.

It seems simpler to preserve the intrinsic until isel, at least for now.

I missed this comment earlier, but that's a valid point. For SVE having side-effects is assumed from the non-faulting flag. We hoped to latch on to the MLOAD here to reuse code and benefit from legalization in case we want to add a more generic mechanism in the future to use such loads directly in the loop-vectorizer.

LGTM! Thanks @c-rhodes

LGTM, thanks @kristof.beyls!

There is no harm in adding these intrinsics, but it is worth pointing out that our downstream compiler does not have explicit intrinsics for the immediate forms because:

• There are no specific C/C++ intrinsics that require the operand to be an immediate. That means using the immediate form is purely an optimisation of the general ((unpredicated) vector, vector) case.
• We can use a pattern to match e.g. the AArch64dup (SVELogicalImm32 i64:$imm) and emit the immediate-form of the instruction.

LGTM!

LGTM!

LGTM!

LGTM!

The patch looks structurally fine, but I'm missing the argumentation for changing the default and related, the history of why the default is to not omit the frame pointer on leaf functions.
Can you provide some insight here?

• Added convenience function MemoryLocation::getSizeOrUnknown
• Removed isLE predicate from predicate store patterns.
• Added comments explaining why we bail out of a function when the type is a scalable vector.
• Addressed other suggestions to clean up code.

Thanks for adding these patterns @cameron.mcinally !

Aside from two nits, LGTM!

LGTM!

LGTM, thanks @efriedma!

Thanks @logan , LGTM!

Thanks for the fixes @fpetrogalli, LGTM!

Functionally the patch is looking good, nearly there!

Seems like a sensible change to me.

LGTM, thanks @c-rhodes!

LGTM!

LGTM!

Thanks @kmclaughlin , LGTM.

Thanks for making these changes to your patch @dancgr.
Please address the two nits before you commit, but otherwise LGTM.