All getReductionCost() functions are renamed to getArithmeticReductionCost() + added basic infrastructure to handle non-binary reduction operations.
Details
Diff Detail
- Build Status
Buildable 7064 Build 7064: arc lint + arc unit
Event Timeline
lib/Analysis/CostModel.cpp | ||
---|---|---|
196 | Can this ever be a Value that's not an Instruction? | |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4221 | 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? | |
4323 | 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? | |
4324 | What does being valid mean in this context? Just that this is a "non-null handle" | |
4371 | only adds at this point. :-) | |
4375 | 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? | |
4397 | 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 | ||
---|---|---|
222 | 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. | |
418 | Check if RdxOp, or use cast instead of dyn_cast? | |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4221 | V is-surely-a<BinaryOperator>, being a non-nullptr to BinaryOperator, right? |
lib/Analysis/CostModel.cpp | ||
---|---|---|
196 | Of course no, only instructions can be used here. I'll convert these params to Instruction *. | |
222 | 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. | |
418 | Yes, missed this, thanks. Will fix. | |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4221 | Yes, you're right, I'll remove these changes from this patch and prepare another one for CmpInsts. | |
4221 | Yes, forgot to change type of parameter, will do it | |
4323 | 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. | |
4324 | Will try to remove it | |
4371 | Yes, will fix this. | |
4375 | Yes, it is exactly what I meant. | |
4397 | Ok, will fix it. |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4339 | Should IsReducedValue be set to false here? | |
4375 | 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. | |
4393 | re[d]uction |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4221 | 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. | |
4322 | "+ validity" should be removed? | |
4345 | Still a couple of "if (x.Opcode == 0)" cases, may as well make use of this bool(). | |
4371 | 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? | |
4375 | 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? | |
4393 | typo | |
4395 | 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? | |
4910 | Not sure why you wanted this change; but how about simply "if (isa<BinaryOperator>(Inst))"? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
4221 | Restored back. | |
4322 | Removed | |
4345 | Tried to replace all | |
4371 | Added isVectorized(Instruction *I) | |
4375 | Modified it a bit, still need to compare the opcode | |
4393 | Yes, thanks, fixed | |
4395 | Modified this method, now it accepts only Builder and optional name | |
4910 | Restored it |
lib/Analysis/CostModel.cpp | ||
---|---|---|
189 | Contains |
Some minor style comments
lib/Analysis/CostModel.cpp | ||
---|---|---|
202 | No need to initialize these. Value *L, *R; | |
204 | Use the ReductionData constructor for clarity? | |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
4354 | ? | |
4359 | Add assert message | |
4364 | Add assert message | |
4369 | Add assert message | |
4433 | Add assert message | |
4450 | Use OperationData constructor for clarity |
A couple of NFC changes that will reduce this patch.
include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
1121 | 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 | 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 | ||
---|---|---|
4339 | 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.
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.