All getReductionCost() functions are renamed to getArithmeticReductionCost() + added basic infrastructure to handle non-binary reduction operations.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Analysis/CostModel.cpp | ||
---|---|---|
196 ↗ | (On Diff #86662) | Can this ever be a Value that's not an Instruction? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4094 ↗ | (On Diff #86662) | This basically complements the change in line 4803 - 4823, right, but taken together, those two changes are NFC, regardless of everything else here, right? Or am I confused and this depends on something else? |
4197 ↗ | (On Diff #86662) | This part of the change is completely orthogonal to the name change and using matchers, right? Anyway, I think I see where this is going. This is possibly similar to what Ayal, Gil & company are doing for LV, in terms of recipes? |
4198 ↗ | (On Diff #86662) | What does being valid mean in this context? Just that this is a "non-null handle" |
4245 ↗ | (On Diff #86662) | only adds at this point. :-) |
4249 ↗ | (On Diff #86662) | So, by "of the same kind" you mean "both a reduction op or both a reduced value", which is something you determine by either both LHS/RHS being null, or both being non-null? |
4271 ↗ | (On Diff #86662) | I've cleaned this up in the existing code, there's no need to special-case FAdd, you can just CreateBinOp for it. |
lib/Analysis/CostModel.cpp | ||
---|---|---|
211 ↗ | (On Diff #86833) | Can PatternMatch be put to more use, by asking it to match a BinOp whose L and R sides are both shuffles, when Level > 0? Passing L and R by reference here saves little compared to the original direct calls to getOperand(0) and getOperand(1), and seems mostly redundant elsewhere. |
405 ↗ | (On Diff #86833) | Check if RdxOp, or use cast instead of dyn_cast? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4094 ↗ | (On Diff #86833) | V is-surely-a<BinaryOperator>, being a non-nullptr to BinaryOperator, right? |
lib/Analysis/CostModel.cpp | ||
---|---|---|
211 ↗ | (On Diff #86833) | This is just an initial patch. In future it will be extended with support of min/max patterns, that's why this kind of recognition of LHS/RHS operands is placed in function. Another thing, that it will be better just to return the record of opcode + LHS/RHS parts, rather than pass L/R by reference. |
405 ↗ | (On Diff #86833) | Yes, missed this, thanks. Will fix. |
196 ↗ | (On Diff #86662) | Of course no, only instructions can be used here. I'll convert these params to Instruction *. |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4094 ↗ | (On Diff #86833) | Yes, forgot to change type of parameter, will do it |
4094 ↗ | (On Diff #86662) | Yes, you're right, I'll remove these changes from this patch and prepare another one for CmpInsts. |
4197 ↗ | (On Diff #86662) | No, actually it is basic infrastructure for supporting horizontal reduction vectorization of not-BinOp instructions. It does not change the functionality, but allows later easily to support vectorization of other complex instructions, like min/max. |
4198 ↗ | (On Diff #86662) | Will try to remove it |
4245 ↗ | (On Diff #86662) | Yes, will fix this. |
4249 ↗ | (On Diff #86662) | Yes, it is exactly what I meant. |
4271 ↗ | (On Diff #86662) | Ok, will fix it. |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4213 ↗ | (On Diff #86833) | Should IsReducedValue be set to false here? |
4267 ↗ | (On Diff #86833) | re[d]uction |
4249 ↗ | (On Diff #86662) | The IsReducedValue explicit flag indicates if its a reduction op or reduced value, right? To implement the behavior documented above: "Checks if two operation data are both a reduction op or both a reduced value", it's enough to have the "IsReducedValue == OD.IsReducedValue" part. |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4262 ↗ | (On Diff #88877) | "+ validity" should be removed? |
4285 ↗ | (On Diff #88877) | Still a couple of "if (x.Opcode == 0)" cases, may as well make use of this bool(). |
4865 ↗ | (On Diff #88877) | Not sure why you wanted this change; but how about simply "if (isa<BinaryOperator>(Inst))"? |
4094 ↗ | (On Diff #86833) | I must be missing something here; tryToVectorize() is still being fed by BinaryOperator*, right? Wondering why the parameter type change. Better include this change in follow-up patch which tries to vectorize non BinaryOperators, if that's the intention. |
4267 ↗ | (On Diff #86833) | typo |
4269 ↗ | (On Diff #86833) | Every other method fits with the passive, storage-type "OperationData" name of this struct, but this last method may look a bit odd doing "OperationData.createOp()". All that it's contributing is its Opcode which it can provide, and some asserts. Is there a better way of keeping OperationData's nature, or a better name for it? |
4245 ↗ | (On Diff #86662) | isVectorizable() is used only in conjuction with isAssociative(I), and as an assert when getOpcode() is called by getReductionCost(). Isn't it better to have isVectorizable(I) also call isAssociative? |
4249 ↗ | (On Diff #86662) | It sounds like you can assert ((IsReducedValue != OD.IsReducedValue) || ((!LHS == !OD.LHS) && (!RHS == !OD.RHS) && Opcode == OD.Opcode) && "message"); no need to actually check them all to return true, right? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4262 ↗ | (On Diff #88877) | Removed |
4285 ↗ | (On Diff #88877) | Tried to replace all |
4865 ↗ | (On Diff #88877) | Restored it |
4094 ↗ | (On Diff #86833) | Restored back. |
4267 ↗ | (On Diff #86833) | Yes, thanks, fixed |
4269 ↗ | (On Diff #86833) | Modified this method, now it accepts only Builder and optional name |
4245 ↗ | (On Diff #86662) | Added isVectorized(Instruction *I) |
4249 ↗ | (On Diff #86662) | Modified it a bit, still need to compare the opcode |
lib/Analysis/CostModel.cpp | ||
---|---|---|
189 ↗ | (On Diff #101209) | Contains |
Some minor style comments
lib/Analysis/CostModel.cpp | ||
---|---|---|
202 ↗ | (On Diff #101209) | No need to initialize these. Value *L, *R; |
204 ↗ | (On Diff #101209) | Use the ReductionData constructor for clarity? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4353 ↗ | (On Diff #101209) | ? |
4358 ↗ | (On Diff #101209) | Add assert message |
4363 ↗ | (On Diff #101209) | Add assert message |
4368 ↗ | (On Diff #101209) | Add assert message |
4394 ↗ | (On Diff #101209) | Add assert message |
4445 ↗ | (On Diff #101209) | Use OperationData constructor for clarity |
A couple of NFC changes that will reduce this patch.
include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
1121 ↗ | (On Diff #101750) | You should probably move this comment (unchanged) as a NFC commit right away to reduce the patch. I can't tell if you've changed anything. |
include/llvm/Transforms/Vectorize/SLPVectorizer.h | ||
87 ↗ | (On Diff #101750) | Do this as an NFC commit now - its a tidyup that isn't relevant to the patch. |
There's still a lot going on in this patch but it looks alright to me (I noticed one minor).
Does anyone have any further comments?
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4337 ↗ | (On Diff #108463) | If OperationData is a struct then this public can go? |
Could we split it on 2 sub-parts:
1 - rename (maybe NFC?)
2 - new infrastructure
In this case it will be easier and smaller.
LGTM - if possible please commit the getArithmeticReductionCost rename as a separate pre-commit.