This is an archive of the discontinued LLVM Phabricator instance.

[X86] Reenable store merging post-legalization
AcceptedPublic

Authored by lebedev.ri on Jan 14 2023, 1:25 PM.

Details

Summary

D62498 disabled store merging post-legalization,
because we do indeed want to split store of concat.

But the current way it's disabled is a way too heavy of a hammer.
We only want to disable the cases that would be problematic,
but e.g. still want to allow store merging of loads.

This requires D141776, otherwise we have conflicting combines.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 14 2023, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2023, 1:25 PM
lebedev.ri requested review of this revision.Jan 14 2023, 1:25 PM
lebedev.ri planned changes to this revision.
lebedev.ri abandoned this revision.Jan 14 2023, 1:27 PM

(wrong diff)

Now with a proper diff.

lebedev.ri added inline comments.Jan 14 2023, 2:06 PM
llvm/lib/Target/X86/X86ISelLowering.h
1074–1075

This is a bit ugly. I'm using StoreSource::Unknown as
"we are post-legalization. do we allow merging *any* sources?".
Is that too ugly?

pengfei added inline comments.Jan 15 2023, 3:22 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19397–19398 ↗(On Diff #489316)

If I read the patch correctly, this is the real point we care about. Should it be better to just introduce another TLI function to do such work?

lebedev.ri marked an inline comment as done.

Don't abuse StoreSource::Unknown, wrap it into std::optional<>.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19397–19398 ↗(On Diff #489316)

I don't see why? We are simply enriching it with more knowledge to give a more precise answer.

barannikov88 added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
606 ↗(On Diff #489349)

Appears unused with std::optional.

llvm/lib/Target/X86/X86ISelLowering.h
1074–1075

Just always pass "valid" StoreSrc as determined by getStoreSource?

lebedev.ri marked 2 inline comments as done.Jan 15 2023, 6:27 AM
lebedev.ri added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
606 ↗(On Diff #489349)

I do not follow. Both the struct and the element are both clearly still used.

llvm/lib/Target/X86/X86ISelLowering.h
1074–1075

The idea is that there is no point in ever trying to guess the StoreSource,
which may be costly to do, if the target will always refuse.

barannikov88 added inline comments.Jan 15 2023, 6:34 AM
llvm/include/llvm/CodeGen/TargetLowering.h
606 ↗(On Diff #489349)

Yes, my bad. Although mergeStoresAfterLegalization will never receive Unknown.
I'd suggest to stick to either Unknown or std::optional, but not both. Either is fine to me.

lebedev.ri marked 3 inline comments as done.Jan 15 2023, 6:42 AM
lebedev.ri added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
606 ↗(On Diff #489349)

Note that i'm simply moving the existing enum, it can't not have all of it's original elements.

lebedev.ri marked an inline comment as done.Jan 18 2023, 12:32 PM

ping

pengfei added inline comments.Jan 18 2023, 8:57 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
674–675 ↗(On Diff #489349)

I'd agree with @barannikov88. Unknown and optional represent the same meaning. Why don't return std::nullopt here too?

lebedev.ri marked an inline comment as done.Jan 19 2023, 5:14 AM
lebedev.ri added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
674–675 ↗(On Diff #489349)

The literally next change will return splat here instead.

lebedev.ri marked an inline comment as done.

Rebased, NFC.
Now that D141776 has landed, this can proceed.

RKSimon added a subscriber: TokarIP.

CC'ing @TokarIP who recently added the light-avx tuning flag for something similar.

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h
247 ↗(On Diff #491099)

drop unused StoreSrc?

mergeStoresAfterLegalization(EVT, std::optional<StoreSource>)
llvm/lib/Target/X86/X86ISelLowering.h
1076

should we play safe and treat unknown as a potential extractsubvector?

lebedev.ri added inline comments.Jan 22 2023, 4:58 AM
llvm/lib/Target/X86/X86ISelLowering.h
1076

Let me just assert that we don't get unknown here.

lebedev.ri marked 2 inline comments as done.

@RKSimon thank you for taking a look!
Addressing nits.

TokarIP accepted this revision.Jan 25 2023, 2:39 PM
This revision is now accepted and ready to land.Jan 25 2023, 2:39 PM