- User Since
- Oct 21 2016, 1:19 AM (221 w, 6 d)
Tue, Jan 19
LGTM, thanks @rogfer01!
Mon, Jan 18
- Reordered includes.
- Changed two more variables to be of type InstructionCost instead of int.
Sun, Jan 17
Thanks for the latest changes, the patch looks good to me now!
Fri, Jan 15
Thu, Jan 14
Added two nits, but LGTM otherwise.
Thanks for the changes so far, I think the patch is nearly there now.
Wed, Jan 13
Tue, Jan 12
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!
Mon, Jan 11
Fri, Jan 8
Thu, Jan 7
LGTM, but it's not clear to me if @ctetreau's latest comment still needs to be addressed.
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!
Wed, Jan 6
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.
Tue, Jan 5
Great suggestion, I've added the assert!
Added test with different start value for fadd reduction.
Mon, Jan 4
- Rebased patch
- Use IMPLICIT_DEF for second source operand to faddp.
- 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.
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.
Dec 13 2020
This change is purely mechanical. LGTM
Dec 11 2020
@jmorse, would you be happy to formally accept the patch?
Dec 10 2020
Dec 9 2020
Thanks for the review @jmorse!
- Added extra CHECK line and TODO to test.
Dec 8 2020
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.
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!