This is an archive of the discontinued LLVM Phabricator instance.

[MergedLoadStoreMotion] Merge stores with conflicting value types
ClosedPublic

Authored by jrbyrnes on Mar 31 2023, 12:05 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jrbyrnes created this revision.Mar 31 2023, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 12:05 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jrbyrnes requested review of this revision.Mar 31 2023, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 12:05 PM
nikic requested changes to this revision.Mar 31 2023, 12:25 PM

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.

This revision now requires changes to proceed.Mar 31 2023, 12:25 PM

Also test pointers with different address spaces

jrbyrnes updated this revision to Diff 510636.Apr 3 2023, 4:09 PM

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.

nikic requested changes to this revision.EditedApr 4 2023, 5:05 AM

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.

This revision now requires changes to proceed.Apr 4 2023, 5:05 AM
jrbyrnes updated this revision to Diff 510826.Apr 4 2023, 8:56 AM
jrbyrnes marked 4 inline comments as done.

Expose hasSameSpecialState to replace isSameLoadStoreOperationAs

arsenm added inline comments.Apr 4 2023, 9:01 AM
llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
266

This should not introduce addrspacecast. I’m assuming this is filtered by isBitOrNoopPointerCastable

jrbyrnes marked an inline comment as done.Apr 4 2023, 9:22 AM
jrbyrnes added inline comments.
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)

nikic accepted this revision.Apr 4 2023, 9:52 AM

LGTM

llvm/include/llvm/IR/Instruction.h
774

opcde -> opcode

I also suspect this doesn't follow clang-format due to overlong comments.

This revision is now accepted and ready to land.Apr 4 2023, 9:52 AM
jrbyrnes updated this revision to Diff 510853.Apr 4 2023, 10:31 AM
jrbyrnes marked 2 inline comments as done.

Comment & clang-format