- User Since
- Nov 20 2019, 6:41 AM (53 w, 1 d)
- Fixed shared library build issue due to InstructionCost::print living in llvm/lib/Analysis. I've moved this to a new file - llvm/lib/Support/InstructionCost.cpp - which resolves dependency issues.
- Removed isInvalid() in favour of getState() that is more future-proof.
- Converted "" comments to "/"
OK, so it is possible to check for validity just using getValue() by writing code such as:
I think the main reason for having isValid (or something like that) was because it sometimes made control flow easier and there are places in the codebase today where all we want to do is assert that something is valid. I guess the alternative to calling isValid is something like:
- Rebased off parent patch.
- Changed calls to isInvalid to !isValid instead, since I feel this is more future-proof in case additional states are added to the InstructionCost class.
- Added documentation to getOptionalElementCountLoopAttribute and simplified the implementation in the function.
- Removed some tbaa metadata from the tests.
- Added tests for value extraction.
Tue, Nov 24
- Changed getValue() to return an Optional<CostType>
Mon, Nov 23
Hi @fhahn @SjoerdMeijer @paulwalker-arm, are you happy with the patch now? The implementation on the LLVM IR side of things is independent of the clang pragma patch. For the clang patch I intend to write a proposal to the mailing list for changing the vectorize_width #pragma. @fhahn would you be happy with removing the "Request changes" cross?
- Updated the documentation to mention that the new attribute only has any effect if vectorisation is enabled for the loop.
- Cleaned up one of the test files.
Fri, Nov 20
Wed, Nov 18
- Added comments explaining the reason for the behaviour used in the comparison operators.
- Removed the comment above operator-.
- Added private propagateState() function, which is more generic and will support more than state. It uses the same lexicographical ordering as the comparisons.
- Updated the tests to use more readable variable names.
Hi @paulwalker-arm, so the reason for this change is related to the child patch where we changed the vectorize_width #pragma to accept an additional optional argument "scalable" or "fixed". @SjoerdMeijer felt this was the wrong approach and that the scalable property should be specified only as an additional pragma, i.e.
After discussions on the child patch (D89031) we've decided to do things differently so instead of adapting vectorize.width to accept a tuple we're now adding a new vectorize.scalable.enable hint that can be used with vectorize.width to create a ElementCount. For the clang patch I also intend to update it so that we use a new vectorize_style #pragma instead of using vectorize_width to force scalable vectorisation.
- Reverted changes to vectorize.width loop hint attribute.
- Added new vectorize.scalable.enable loop hint attribute to control scalable vectorisation.
Tue, Nov 17
Fri, Nov 13
- Changed comparison operators to explicitly treat invalid as infinitely expensive and return the appropriate result.
- Added setValid() function.
- Changed enum states to Valid and Invalid.
- Removed template from the InstructionCost class and defaulted to 'int'. I think 32 bits is enough for costs, since everywhere in the codebase uses int or unsigned. However, the cost type can be easily changed since I made it a typedef. Not having the template meant I could also simplify the list of operators.
- Changed lib/Transforms/Scalar/CallSiteSplitting.cpp so that we no longer explicitly check isInvalid() before comparing with DuplicationThreshold. This is because the ">=" operator now takes the invalid state into account.
Thu, Nov 12
Hi @SjoerdMeijer I think that given we now support scalable vectors we thought it made sense to be able to specify whether the user wants 'fixed' or 'scalable' vectorisation along with the vector width, although without specifying the additional property the default continues to remain 'fixed'. However, what you said about having a vectorize_scalable pragma is correct and we are intending to also add a pragma like this in a future patch.
Wed, Nov 11
- In LoopVectorize.cpp I've forced the UserVF to fall back on fixed width vectorisation for now.
- Added comments to the new test file and renamed it.
Tue, Nov 10
Mon, Nov 9
- Added more comments documenting the algorithm in AArch64CallingConvention.cpp.
- Rewrote loops in AArch64ISelLowering.cpp
Fri, Nov 6
- Fixed up some remaining vectorize.width attribute cases.
Thu, Nov 5
Wed, Nov 4
Tue, Nov 3
Hi @fhahn are you happy with the changes now and happy to accept them?
Fri, Oct 30
LGTM and a lot tidier now, provided you're also happy with this @ctetreau?
Thu, Oct 29
- Updated documentation.
- Addressed review comments.
- Added fatal errors on the callee side when one of the variadic arguments is a scalable vector.
Oct 27 2020
Is this the right way to go here? I've assumed we want to remove the operators, but perhaps they should be marked as deprecated and left in for a while first?
Oct 26 2020
Oct 23 2020
One suggestion for a possible fold of some additions, but otherwise looks good to me!
Oct 21 2020
Oct 20 2020
Oct 19 2020
Is it perhaps worth renaming the test to have sve in the name, i.e. sve-redundant-stores.ll? We've been following this convention for other tests.
- Updated documentation for vectorize_width loop attribute.
- Added lambda functions to be used when validating and setting loop hint attributes.
Oct 16 2020
Oct 15 2020