Details
- Reviewers
dblaikie • tstellarAMD mjacob
Diff Detail
Event Timeline
lib/CodeGen/AtomicExpandPass.cpp | ||
---|---|---|
271 | I must be missing something as this doesn't look quite right - why was the original code treating the load as though it were a load of a pointer? (load's IR type should be a value type, not a pointer type - unless it is loading a pointer (ie: load could be passed a pointer to a pointer)) & why is the new code doing something different? | |
lib/Transforms/Vectorize/BBVectorize.cpp | ||
993 | I take it this code is still correct - but you chose to plumb the two out params into getPairPtrInfo just so this code didn't have to do all the casting and conditionals, etc? (since you did have to add JTy/ITy to the implementation of getPairPtrInfo anyway, yes?) | |
2317 | Same here, I take it? Or was this the part where it was necessary for getPairPtrInfo to produce these two values - and since you were producing them here you might as well use them in the other call site? | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
1991 | Do we need to support a null type parameter here? |
lib/CodeGen/AtomicExpandPass.cpp | ||
---|---|---|
271 | getNullValue, confusingly enough, isn't about nullptr but zeroinitializer. | |
lib/Transforms/Vectorize/BBVectorize.cpp | ||
2317 | Correct - otherwise I would've ended up with unused variables and duplicated work. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
1991 | This might be an artifact of rebasing, i.e. it was needed but now it's not. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
1991 | Ah, that call site doesn't show up in the commit because it hasn't changed. |
lib/Target/PowerPC/PPCLoopPreIncPrep.cpp | ||
---|---|---|
209 | I think you changed semantics here. What if the instruction is a prefetch intrinsic with a pointer to a vector as the first operand? | |
lib/Transforms/IPO/GlobalOpt.cpp | ||
2437 | Do we need this variable? | |
lib/Transforms/Scalar/MemCpyOptimizer.cpp | ||
236 | I think it's slightly more robost to get the type with MSI->getValue()->getType(). | |
lib/Transforms/Scalar/Scalarizer.cpp | ||
268–270 | Can you document (and possibly add an assertion for) the requirements on VecTy? If I understand correctly, there are two possibilities:
At least we should add an assertion that V is a vector if VecTy is nullptr. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
1991 | What if isConsecutivePtr is called from collectLoopUniforms with a pointer to a struct? |
lib/Target/PowerPC/PPCLoopPreIncPrep.cpp | ||
---|---|---|
209 | Prefetch is monomorphic, it only takes one pointer, AFAICT. | |
lib/Transforms/IPO/GlobalOpt.cpp | ||
2437 | No, although, I might put SI->getValueOperand() in a variable just to keep the line length in check. | |
lib/Transforms/Scalar/Scalarizer.cpp | ||
268–270 | I agree with that - I also need to extract the element type and use it, which for some reason I didn't do back when I first made this change. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
1991 | Honestly, I'm not sure. I don't think that it affects the resulting decisions, but I can try to look deeper into it. |
I must be missing something as this doesn't look quite right - why was the original code treating the load as though it were a load of a pointer? (load's IR type should be a value type, not a pointer type - unless it is loading a pointer (ie: load could be passed a pointer to a pointer)) & why is the new code doing something different?