Page MenuHomePhabricator

[opaque pointer types] [NFC] {Load,Store}Inst: get loaded/stored type from the instruction.
Needs ReviewPublic

Authored by eddyb on Jan 21 2016, 10:43 AM.

Diff Detail

Event Timeline

eddyb updated this revision to Diff 45563.Jan 21 2016, 10:43 AM
eddyb retitled this revision from to [opaque pointer types] [NFC] {Load,Store}Inst: get loaded/stored type from the instruction..
eddyb updated this object.
eddyb added reviewers: mjacob, dblaikie.
eddyb added a subscriber: llvm-commits.
dblaikie added inline comments.Jan 21 2016, 3:09 PM
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?

eddyb added inline comments.Jan 21 2016, 3:26 PM
lib/CodeGen/AtomicExpandPass.cpp
271

getNullValue, confusingly enough, isn't about nullptr but zeroinitializer.
And I didn't change the semantics, I just replaced LI->getPointerOperand()->getPointerElementType() with LI->getType().

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.
I'll try testing without the nullptr default and this check.

eddyb added inline comments.Jan 21 2016, 3:33 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
1991

Ah, that call site doesn't show up in the commit because it hasn't changed.
See the collectLoopUniforms method.

mjacob added inline comments.Jan 24 2016, 11:38 AM
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:

  1. V is a vector and VecTy is nullptr.
  2. V is a pointer to a vector and VecTy is the pointee type.

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?

eddyb added inline comments.Jan 24 2016, 1:31 PM
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.