LGTM

LGTM, thanks @rogfer01!

LGTM

LGTM!

• Reordered includes.
• Changed two more variables to be of type InstructionCost instead of int.

Thanks for the latest changes, the patch looks good to me now!

Gentle ping

Added two nits, but LGTM otherwise.

Thanks for the changes so far, I think the patch is nearly there now.

In D94444#2492855, @cameron.mcinally wrote:

I like that too. I also like David's suggestion about deinterleave.even. I don't really have a strong opinion on either though. Does anyone feel strongly either way?

I quite like Dave's suggestion for deinterleave.even/odd and interleave.lo/hi, because now they are related (antonyms) and the names are still intuitive.

Thanks for creating this patch!

LGTM

LGTM

LGTM, but it's not clear to me if @ctetreau's latest comment still needs to be addressed.

In D92238#2480072, @ctetreau wrote:

In D92238#2452418, @lebedev.ri wrote:

I think this is conflating use-cases.
I'm not sure the entire InstructionCost should be used as a variable to track full/remaining cost over a several instructions.
Perhaps there should be a[nother] abstraction for that?

Really, we were already doing this; we just didn't have a special cost type. We could leave the cost as an integer, but then the code would just become way more verbose (unwrapping every InstructionCost after computing them). Using the metaphor of money, a cost is a quantity of money. I think it's reasonable to subtract money from a budget. I suppose we could accumulate a running cost, and compare it with the budget that remains an integer, but that would just introduce another variable for (IMO) no good reason.

Like @ctetreau I don't see why the algorithm shouldn't be using InstructionCost. The class has enough capabilities that any invalid state now propagates quite naturally without too big changes (InstructionCost behaves mostly like a scalar int that carries extra state).
I've simplified the use of *BudgetRemaining.getValue() < 0 to BudgetRemaining < 0, which hopefully makes the patch more palatable?

• Simplified *BudgetRemaining.getValue() < 0 to BudgetRemaining < 0.
• Changed int Cost; to InstructionCost Cost.
• Rebased patch.

LGTM, thanks for all the changes @david-arm!

LGTM, thanks for your work on this patch @c-rhodes!

This change should be mentioned in the LangRef as well, because that currently specifies:

scalable vectors cannot be global variables or members of structs or arrays because their size is unknown at compile time.

LGTM

LGTM

In D90020#2478868, @dstenb wrote:

Since the prependOffsetExpression function is outside DIExpression perhaps it is worth just adding an assert that catches unknown flags?

assert((PrependFlags & ~(DerefBefore | DerefAfter | StackValue | EntryValue)) == 0 && "Unknown prepend flag");

Great suggestion, I've added the assert!

Added assert.

Added test with different start value for fadd reduction.

• Rebased patch
• Use IMPLICIT_DEF for second source operand to faddp.

In D59259#2476240, @RKSimon wrote:

@sdesmalen whatever happened to this patch?

• Rebased patch (after changes to D90020 it can use getOffsetOpcodes directly, instead of adding a new function appendOffsetExpr that does the same thing)
• Removed unnecessary attributes from the test.

In D90020#2455122, @dstenb wrote:

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

Thanks for the suggestion, you made a good point that this interface was error prone when new flags are added in the future. I've updated the patch and added the virtual interface getOffsetOpcodes alongside prependOffsetExpression (made non-virtual and now handles the Deref, Stack/Entry Value). Let me know if this is how you imagined it.

• Made TargetRegisterInfo::prependOffsetExpression non-virtual and changed it to handle the Prepend flags.
• Added virtual method TargetRegisterInfo::getOffsetOpcodes, which gets called by prependOffsetExpression.

LGTM with nits addressed and TODO removed.

This change is purely mechanical. LGTM

LGTM

In D90020#2436998, @jmorse wrote:

On the whole, LGTM, but I'd wait a bit to see if @aprantl is happy with the explanation. (Adding debug-info group too).

@jmorse, would you be happy to formally accept the patch?

LGTM

Thanks for the review @jmorse!

• Added extra CHECK line and TODO to test.

Are there any scenarios where stack slots can be accessed with different scalable offsets? For example if stack slot colouring merged two slots for vectors of different scalable size (maybe it doesn't do that). If that happened, the StackOffset object would compare differently, and we might miss a spilt value being overwritten (see llvm/test/DebugInfo/MIR/X86/live-debug-values-stack-clobber.mir for a scalar example of overwrites).

I guess that is possible. As long as both frame objects have the same Stack-ID, StackSlotColouring can merge them together. However, in that case, the sizes and offsets of those objects would both be scalable and comparing these sizes would be no different as they would be for non-scalable sizes.

LGTM!

In D92237#2420530, @lebedev.ri wrote:

I don't like this.
I intentionally made this change from unsigned to signed back in D67318,
and then followed the same pattern when re-writing isHighCostExpansionHelper(),
because using signed is that much more easy to catch budget overrun
as opposed to basically ensuring that each subtraction doesn't cause an overflow
(or failing to do that, and having an infinite budget)

LGTM!

Rebased patch after D90687 and D88962 landed. This required a small change to metadata-width.ll, because the original test case tries to vectorize an induction variable, which this prototype doesn't yet support.

Thanks for adding the tests and fixing the warnings, LGTM!