User Details
- User Since
- Oct 21 2016, 1:19 AM (234 w, 4 d)
Today
Is it worth adding a test to llvm/unittests/IR/IRBuilderTest.cpp for this change?
LGTM!
Yesterday
Other than my comment on two missing tests, the patch looks good to me.
LGTM, thanks @CarolineConcatto
Fri, Apr 16
Changed back to having isMoreProfitable in LV CostModel.
Thu, Apr 15
Thanks for the changes @CarolineConcatto
Thanks, that looks quite neat. Can you also add a few tests when there is >1 use of the stepvector (e.g. using the stepvector in two adds), so we can test the fold indeed doesn't happen?
One test for each of the instructions should be sufficient.
Wed, Apr 14
Can you also add support for predicates?
Removed another case of .getValue()
Tue, Apr 13
- Addressed nits.
- s/MaxVectorSize/MaxVectorElementCount/ (because it's not actually a size, but an element count).
There isn't a firm rule to follow re tablegen pattern vs manual dag combine, but I think the steer is that when the combine is needed after legalization and it's possible to specify as a pattern, a pattern is preferred. In this case, probably a pattern would be a clean way to implement it. I haven't used SDNPCommutative before, but that looks like it should do the trick.
It's not entirely clear what original intent of this function argument was since the doxygen comment doesn't describe it.
If the argument also isn't used anywhere then it seems sensible to me to remove it, so LGTM.
Mon, Apr 12
Thanks for the review @dmgreen!
After reading the summary/intent of the patch, I thought the same thing as @rjmccall. Simply returning an i32 for the above example and removing the rounding-up seems right to me.
Removed unnecessary .getValue()
Addressed review comments.
Merged patch with D100207
Thanks for taking time from your weekend to look into this @tambre, much appreciated!
Removed forgetBuiltin and the code using it.
Fri, Apr 9
Revert InstructionCost change for Num variable.
Thanks, LGTM!