-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[DAGCombiner] Move handling of atomic loads from SystemZ to DAGCombiner (NFC). #86484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1454,6 +1454,28 @@ class TargetLoweringBase { | |
getLoadExtAction(ExtType, ValVT, MemVT) == Custom; | ||
} | ||
|
||
/// Same as getLoadExtAction, but for atomic loads. | ||
LegalizeAction getAtomicLoadExtAction(unsigned ExtType, EVT ValVT, | ||
EVT MemVT) const { | ||
if (ValVT.isExtended() || MemVT.isExtended()) return Expand; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line break There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is copied from getLoadExtAction() a few lines up in the file, so maybe do the same..? (Or reformat both, I guess) |
||
unsigned ValI = (unsigned)ValVT.getSimpleVT().SimpleTy; | ||
unsigned MemI = (unsigned)MemVT.getSimpleVT().SimpleTy; | ||
assert(ExtType < ISD::LAST_LOADEXT_TYPE && ValI < MVT::VALUETYPE_SIZE && | ||
MemI < MVT::VALUETYPE_SIZE && "Table isn't big enough!"); | ||
unsigned Shift = 4 * ExtType; | ||
LegalizeAction Action = | ||
(LegalizeAction)((AtomicLoadExtActions[ValI][MemI] >> Shift) & 0xf); | ||
assert((Action == Legal || Action == Expand) && | ||
"Unsupported atomic load extension action."); | ||
return Action; | ||
} | ||
|
||
/// Return true if the specified atomic load with extension is legal on | ||
/// this target. | ||
bool isAtomicLoadExtLegal(unsigned ExtType, EVT ValVT, EVT MemVT) const { | ||
return getAtomicLoadExtAction(ExtType, ValVT, MemVT) == Legal; | ||
} | ||
|
||
/// Return how this store with truncation should be treated: either it is | ||
/// legal, needs to be promoted to a larger size, needs to be expanded to some | ||
/// other code sequence, or the target has a custom expander for it. | ||
|
@@ -2536,6 +2558,30 @@ class TargetLoweringBase { | |
setLoadExtAction(ExtTypes, ValVT, MemVT, Action); | ||
} | ||
|
||
/// Let target indicate that an extending atomic load of the specified type | ||
/// is legal. | ||
void setAtomicLoadExtAction(unsigned ExtType, MVT ValVT, MVT MemVT, | ||
LegalizeAction Action) { | ||
assert(ExtType < ISD::LAST_LOADEXT_TYPE && ValVT.isValid() && | ||
MemVT.isValid() && "Table isn't big enough!"); | ||
assert((unsigned)Action < 0x10 && "too many bits for bitfield array"); | ||
unsigned Shift = 4 * ExtType; | ||
AtomicLoadExtActions[ValVT.SimpleTy][MemVT.SimpleTy] &= | ||
~((uint16_t)0xF << Shift); | ||
AtomicLoadExtActions[ValVT.SimpleTy][MemVT.SimpleTy] |= | ||
((uint16_t)Action << Shift); | ||
} | ||
void setAtomicLoadExtAction(ArrayRef<unsigned> ExtTypes, MVT ValVT, MVT MemVT, | ||
LegalizeAction Action) { | ||
for (auto ExtType : ExtTypes) | ||
setAtomicLoadExtAction(ExtType, ValVT, MemVT, Action); | ||
} | ||
void setAtomicLoadExtAction(ArrayRef<unsigned> ExtTypes, MVT ValVT, | ||
ArrayRef<MVT> MemVTs, LegalizeAction Action) { | ||
for (auto MemVT : MemVTs) | ||
setAtomicLoadExtAction(ExtTypes, ValVT, MemVT, Action); | ||
} | ||
|
||
/// Indicate that the specified truncating store does not work with the | ||
/// specified type and indicate what to do about it. | ||
void setTruncStoreAction(MVT ValVT, MVT MemVT, LegalizeAction Action) { | ||
|
@@ -3521,6 +3567,10 @@ class TargetLoweringBase { | |
/// for each of the 4 load ext types. | ||
uint16_t LoadExtActions[MVT::VALUETYPE_SIZE][MVT::VALUETYPE_SIZE]; | ||
|
||
/// Similar to LoadExtActions, but for atomic loads. Only Legal or Expand | ||
/// (default) values are supported. | ||
uint16_t AtomicLoadExtActions[MVT::VALUETYPE_SIZE][MVT::VALUETYPE_SIZE]; | ||
|
||
/// For each value type pair keep a LegalizeAction that indicates whether a | ||
/// truncating store of a specific value type and truncating type is legal. | ||
LegalizeAction TruncStoreActions[MVT::VALUETYPE_SIZE][MVT::VALUETYPE_SIZE]; | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this replace getExtendForAtomicOps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting question.
getAtomicLoadExtAction() is used to check for a specific width / extension type before performing the DAG combine on a load, while getExtendForAtomicOps() is used as a general preference check without any concern of the specific VT, including other things than loads.
During expansion of ATOMIC_CMP_SWAP_WITH_SUCCESS, an ATOMIC_CMP_SWAP is emitted, which may become a libcall it says. Then getExtendForAtomicOps() is used to handle that result, so that is indeed a very general, broad target characteristic, even including a call result.
I guess it looks to me to be more simple to let targets that only have one of these available simply state that by overriding getExtendForAtomicOps(). It will help them everywhere - libcalls, computeKnownBits, ComputeNumSignBits, ... I guess such a target have either the real instruction or a pseudo implemented to do the extension, for all VTs.
It may be possible to have a target instead of doing that, specify all the available legal atomic load extensions. From these, it could then be deduced that e.g. only SIGN_EXTEND is available and a general assumption could be made. But that's not perfect either, so wouldn't be worth the effort, probably...
It should be possible to unify these one way or the other, but it doesn't seem simple to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of getAtomicLoadExtAction I'm more concerned with is the semantic load bearing pieces like this, adding hidden properties to these atomics. It would be better if we just moved the atomic-ness into the MMOs to avoid this magic bit