This is an archive of the discontinued LLVM Phabricator instance.

[X86][AMX] Try to hoist AMX shapes' def
ClosedPublic

Authored by pengfei on Apr 22 2021, 7:21 AM.

Details

Summary

We request no intersections between AMX instructions and their shapes'
def when we insert ldtilecfg. However, this is not always ture resulting
from not only users don't follow AMX API model, but also optimizations.

This patch adds a mechanism that tries to hoist AMX shapes' def as well.
It only hoists shapes inside a BB, we can improve it for cases across
BBs in future. Currently, it only hoists shapes of which all sources' def
above the first AMX instruction. We can improve for the case that only
source that moves an immediate value to a register below AMX instruction.

Diff Detail

Event Timeline

pengfei created this revision.Apr 22 2021, 7:21 AM
pengfei requested review of this revision.Apr 22 2021, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 7:21 AM
pengfei updated this revision to Diff 339635.Apr 22 2021, 7:58 AM

Reset ShapeBBs[MBB] once we hoist any shapes in MBB.

pengfei updated this revision to Diff 339651.Apr 22 2021, 8:19 AM

Always insert before AMX instruction.

xiangzhangllvm added inline comments.Apr 22 2021, 6:40 PM
llvm/lib/Target/X86/X86PreTileConfig.cpp
196–198

ShapeBBs[MBB] may both contains the Row_def and Col_def, and they may be in different BBs, The Pos of them in their BB my be equal, So use lower_bound here seems not good. Because it may duplicated insert X_def at line 201.

xiangzhangllvm added inline comments.Apr 22 2021, 7:13 PM
llvm/lib/Target/X86/X86PreTileConfig.cpp
261

Looks strange here: All BB followed a AMX BB will be mark HasAMXRegLiveIn, Even BBs after the last BB of using AMX?

289

I think "across BBs" is most common case:

AMX BB0 --> shape def --> AMX BB1

pengfei added inline comments.Apr 22 2021, 8:00 PM
llvm/lib/Target/X86/X86PreTileConfig.cpp
196–198

ShapeBBs[MBB] contains Shapes only defined in the same MBB. We don't distinguish Row and Col here.

261

It's true from perspective of the live range of TMM registers. Though might be not precise. (The live range actually ends after function call). We simply use such conception here to check if there's any shape def BBs have AMX registers live in, i.e. the shapes are interact with AMX instructions.

289

We can improve it in follow up patches :)

pengfei updated this revision to Diff 339857.Apr 22 2021, 8:28 PM

Minor refactor.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2021, 9:17 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

I happened to commit this patch with another one. Since Xiang had +1 for it, @LuoYuanke , do you think if I need to revert it or not?

hctim added a subscriber: hctim.Apr 23 2021, 10:42 AM

Looks like this patch broke the MSan buildbots: https://lab.llvm.org/buildbot/#/builders/5/builds/6967/steps/9/logs/stdio

Reproing MSan is a little tricky as it can require a multistage compilation, so you might find the instructions at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild helpful.

pengfei reopened this revision.Apr 24 2021, 8:50 PM

Looks like this patch broke the MSan buildbots: https://lab.llvm.org/buildbot/#/builders/5/builds/6967/steps/9/logs/stdio

Reproing MSan is a little tricky as it can require a multistage compilation, so you might find the instructions at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild helpful.

Thanks @hctim for reverting this and the instructions. I can reproduce it locally.

pengfei updated this revision to Diff 340326.Apr 24 2021, 8:54 PM

Reapply the minor refactor.
Fix MSan build failures.

xiangzhangllvm accepted this revision.Apr 26 2021, 6:48 PM
This revision is now accepted and ready to land.Apr 26 2021, 6:48 PM
This revision was landed with ongoing or failed builds.Apr 26 2021, 7:28 PM
This revision was automatically updated to reflect the committed changes.