Page MenuHomePhabricator

[X86] Sink x86_amx load in AMX type lowering.
Needs ReviewPublic

Authored by LuoYuanke on Dec 1 2020, 7:38 PM.

Details

Summary

When transform x86_amx load to amx load intrinsics, compiler need
to know the shape the matrix. The shape can be deduced from the user
of the load. However the shape may be defined after the load
instruction, so we need to sink the load to avoid such issue. This patch
only support sink within a basic block.

Diff Detail

Event Timeline

LuoYuanke created this revision.Dec 1 2020, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 7:38 PM
LuoYuanke requested review of this revision.Dec 1 2020, 7:38 PM
craig.topper added inline comments.Dec 1 2020, 7:45 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

Does this prevent sinking across a call that may change the memory being loaded?

craig.topper added inline comments.Dec 1 2020, 8:01 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

Or atomics, or anything with side effects.

LuoYuanke added inline comments.Dec 1 2020, 9:50 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

Thank you for review. Yes. I want to prevent all the scenario that may change the memory being loaded. I'll check how many scenario we need to prevent from sinking.

It's better to set parent since the patch is based on D91927.

llvm/lib/Target/X86/X86LowerAMXType.cpp
123–124

Why don't pass the pointer directly for LD and II?

LuoYuanke added inline comments.Dec 2 2020, 9:42 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

@craig.topper , I do some study, but I still don't understand why load can't sink across atomic instruction or side effect instruction. I notice in MergedLoadStoreMotion::isStoreSinkBarrierInRange(), it only check myThrow() and alias.

pengfei added inline comments.Dec 2 2020, 9:59 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

I only noticed we chain the memory operations during building the DAG and these side effect instructions. Do we have such rules for middle end passes?

craig.topper added inline comments.Dec 2 2020, 10:05 PM
llvm/lib/Target/X86/X86LowerAMXType.cpp
100

Doesn't isStoreSinkBarrierInRange call canInstructionRangeModRef not just isnoalias with a range including every instruction between the store and where it wants to be moved to?

"Release" ordering for an atomic means that no load/store can be moved below it, for example.

LuoYuanke updated this revision to Diff 309179.Dec 3 2020, 12:43 AM

Address Craig and Pengfei's comments.

yubing added a subscriber: yubing.Dec 4 2020, 2:34 AM