Page MenuHomePhabricator

[SimplifyCFG] Skip hoisting common instructions that return token type
ClosedPublic

Authored by ychen on Jul 1 2022, 3:57 PM.

Details

Summary

By LangRef, hoisting token-returning instructions obsures the origin
so it should be skipped. Found this issue while investigating a
CoroSplit pass crash.

Diff Detail

Event Timeline

ychen created this revision.Jul 1 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 3:57 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ychen requested review of this revision.Jul 1 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 3:57 PM
ychen updated this revision to Diff 441817.Jul 1 2022, 4:48 PM

run update_test_checks.py on llvm/test/Transforms/LowerMatrixIntrinsics/transpose-opts.ll

Don't think we have a clear understanding of the transforms that are valid for token types, but this generally looks reasonable to me. We should avoid making token values flow across control flow edges, even if it does not require introducing a phi.

llvm/test/Transforms/LowerMatrixIntrinsics/transpose-opts.ll
975 ↗(On Diff #441817)

Unrelated change? Feel free to commit separately.

llvm/test/Transforms/SimplifyCFG/hoist-skip-token.ll
5

Use update_test_checks please.

37

It should be possible to simplify this test further, e.g. the outer loop structure doesn't really seem relevant here.

ychen updated this revision to Diff 442137.Jul 4 2022, 12:48 PM
  • rebase
  • simplify the test case
  • use update_test_checks.py
ychen marked 2 inline comments as done.Jul 4 2022, 12:48 PM
nikic added inline comments.Jul 4 2022, 12:49 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1477

Ugh, I could have sworn I left a comment here, but apparently I didn't: This check shouldn't be here, it should be in the do/while loop below. Otherwise you'll only check that the first instruction doesn't return a token type, but may still hoist it if it's a later instruction.

ychen added inline comments.Jul 4 2022, 1:02 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1477

Ah, thanks for the remainder. After giving it another thought, I'm curious if the check should be in isIdenticalToWhenDefined to say that any two token-returning instructions are distinct? WDYT?

nikic added inline comments.Jul 4 2022, 2:28 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1477

I think isIdenticalToWhenDefined() is a too generic API for this check. I don't think there's anything generally wrong with merging token-returning calls -- the problem here is that we want to either hoist both the token-producing and token-consuming calls, or neither, and the simplest way to do that is to never hoist them.

ychen updated this revision to Diff 442149.Jul 4 2022, 3:15 PM

Address Nikita's feeback

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1477

Agreed. Thanks.

nikic accepted this revision.Jul 5 2022, 12:18 AM

LGTM

This revision is now accepted and ready to land.Jul 5 2022, 12:18 AM
This revision was landed with ongoing or failed builds.Jul 5 2022, 11:22 AM
This revision was automatically updated to reflect the committed changes.

I think we need to clarify LangRef if this is illegal in general for tokens in general. If it is, we need to explain why. If it isn't, we need an attribute the express the right semantics here.

All LangRef really says at the moment is that a token can't be put behind a phi or select; it doesn't really describe if/when it's legal to merge instructions that produce tokens.

ychen added a comment.Jul 5 2022, 3:14 PM

I think we need to clarify LangRef if this is illegal in general for tokens in general. If it is, we need to explain why. If it isn't, we need an attribute the express the right semantics here.

All LangRef really says at the moment is that a token can't be put behind a phi or select; it doesn't really describe if/when it's legal to merge instructions that produce tokens.

Yes. I also found the token type description could be more clear. I'll send a follow-up LangRef patch to clarify the intent.