LGMT David!
Learning new things every day!

Hey David,
I believe I have some nit. :)

Just in case ...

Hey Kerry,

Thank you for adding me as a reviewer, although I don't think I am the more qualified to approve or not this patch.
But I have a question:
Why is this patch only changing int_aarch64_neon_frintn and not int_aarch64_sve_frintn?
Is there a particular reason to do so?
As you said in the commit message the ISD node for FROUNDEVEN exists now.
If so it would be too much to explain that in the commit message?
Thank you,
Carol

Hey Nash,

Hey @david-arm sorry for the nit in the tests.
Usually happens when I do copy and paste many times.
I've updated the patch and rebased it with the main.

• Fix nit in the .ll tests and LoopVectorize.cpp
• Remote copied getRunTimeVF from LoopVectorize.cpp
• Move CreateVectorReverse implementation from IRBuilder.h to IRBuilder.cpp

Thank you all for the review.
I C&P the function getRunTime from D95139.
If D95139 is merged before this patch I will remove it.
All the other dependencies are already in main, and getRunTimeVF is the only function that this patch needs from D95139.

• Add tests for when vector.reverse needs to reverse a mask
• improve code in LoopVectorize.cpp
• C&P function getRunTimeVF from D95139

Thank you Sander!
My mistake not update main. Thank you for pointing it out.
Now the test is based on the latest main repo.

• return file getIntrinsicInstrCost-vector-reverse.ll with the i1 test

• update the llvm-ir test
• rebase

Thank you @david-arm.
I've removed the pre.header.
It is indeed better.

• change index 'i' in the tests to be 64 bits instead of 32bits

-removing redundant lines in the test

-change the names of the tests to named-vector-shuffle-reverse
-add text in LangRef about the use of experimental.vector.reverse

• change how to get NumElt inside LoopVectorize
• change the index to be 64 bits

Thank you all for the review.
I believe I had addressed your comments.
I've created another patch to compute the cost for vector reverse with scalable vector.
I've removed the cast from int to double in one of the tests, so we don't need cast fixed for this feature.
And the IR builder now has a reverse function for fixed and scalable vectors.

• Move cost function for Shuffle reverse to D93639
• Use getRunTimeVF to compute the runtime vector size for scalable vector
• add tests for fixed vector

-remove non used parameter from the test function broadcast

Thank you @david-arm for the test suggestion.
I also added the support for nxv8bf16.

• add support for nxv8bf16 and simplify .ll test

@Thank you all for the suggestion.
I have added the support for MVT::nxv8bf16 and simplify the test.

• correct style

• add bfloat test and support

• simplify sve broadcast test

-replace test name @slapt_v4f32 by @broadcast_v4f32(

-add original if statement
-move the table cost for scalable types to ShuffleTbl

-remove test for f16 on X86 as they are not legal.

Thank you all for the comments.
I've added tests for -O0 in AArch64 and fixed-width test for X86.
ATM the X86 test has no specific target feature.

-add -O0 and X86 test for vector.reverse

-s/scalabe/scalable

Hey Kerry,
Thank you for this patch.
I found some nit and I have some suggestions about instructionCost.

-fix table gen style for TargetSelectionDAG.td

-fix style on LegalizeIntegerTypes.cpp

-remove AArch64 custom lowering
-move SNode vector_reverse to TargetSelectionDAG.td

It look to me now.

-re-arrange the test
-remove f<operand> tests with integer

• change vector size to 128bits

@Thank you Sander,
I added more tests. Now we have tests for legal and not legal, besides the test with integer for FP operands.

-add missing test for fmax,fadd with integer

-replace (!NewCost.isValid && !OldCost.isvalid()) by !NewCost.isValid()

• remove cost for vector.reduce.{mul"fmul}
• add regression tests for not legal vectors

-remove redundant code
-change getArithmeticReductionCostScalableVectorType to getArithmeticReductionCostSVE
-replace unsigned by int

-remove missed assert

-add missing invalid costs test for scalarizeBinopOrCmp

Hi @ctetreau,
Thank you for the review.
So I removed all the asserts and added the earlier return if both costs are invalid, because in this case it means that the transformation is not cheap.
If only OldCost is invalid I believe we should do nothing, for the same reason I removed the test for !NewCost.isValid().

-remove asserts for OldCost invalid
-add return if both costs are invalid