Since memory does not have an intrinsic type, we do not need to require value type matching on stores in order to sink them. To facilitate that, this patch finds stores which are sinkable, but have conflicting types, and bitcasts the ValueOperand so they are easily sinkable into a PHINode. Rather than doing fancy analysis to optimally insert the bitcast, we always insert right before the relevant store in the diamond branch. The assumption is that later passes (e.g. GVN, SimplifyCFG) will clean up bitcasts as needed.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please add tests for a) one store being ptr and the other i64 and b) one store being i64 and the other { i32, i32 }. The former requires the insertion of inttoptr or ptrtoint casts, the latter is not bitcastable. The correct predicate to use here is CastInst::isBitOrNoopPointerCastable() and then IRBuilder::CreateBitOrPointerCast() to materialize the cast.
It's probably simpler and cleaner to explicitly check this than trying to integrate checks into isSameOperationAs() in a way that makes sense.
Add tests for interesting cases.
Check isBitOrNoopPointerCastable when matching stores.
Combine isLoadStoreConflictingType , createBitCast => CreateBitOrPointerCast
Remove isTriviallyBitcasted (this is aleady caught by store->isSimple() and AA->isMustAlias.
The logic here looks fine to me now. I'm not a big fan of the change to isSameOperationAs(), please see the inline suggestion.
llvm/lib/IR/Instruction.cpp | ||
---|---|---|
599 | With CompareIgnoringType this basically becomes the same as just haveSameSpecialState(). I'd suggest exposing that as a public method rather than adding this flag. | |
llvm/test/Transforms/MergedLoadStoreMotion/st_sink_conflict_type.ll | ||
4 | Probably not needed. | |
6 | fastcc/unnamed_addr not needed | |
32 | Drop the .i.i.i.i suffixes here. |
llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp | ||
---|---|---|
266 | This should not introduce addrspacecast. I’m assuming this is filtered by isBitOrNoopPointerCastable |
llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp | ||
---|---|---|
266 | If the valueOperand of the store are ptrs of conflicting addrspace, they are not sink candidates. isBitOrNoopPointerCastable will return false due to explicit addrspace check (return SrcPtrTy->getAddressSpace() == DestPtrTy->getAddressSpace()). (caught by sink_addrspace test) Potentially relevant is that MLSM finds sink candidates based on matching pointers of stores (S0->getPointerOperand() == S1->getPointerOperand()). So, we won't be able to match on addressspace casted pointers (sink_addrspace3 test) |
LGTM
llvm/include/llvm/IR/Instruction.h | ||
---|---|---|
774 | opcde -> opcode I also suspect this doesn't follow clang-format due to overlong comments. |
opcde -> opcode
I also suspect this doesn't follow clang-format due to overlong comments.