[HLK] Modify Wave match test logic to support modifications in different lanes and vector position #7991
Conversation
| uint64_t LowBits; | ||
| uint64_t HighBits; | ||
|
|
||
| Wave(CONST UINT NumWaves, uint64_t LB, uint64_t HB) { |
There was a problem hiding this comment.
| Wave(CONST UINT NumWaves, uint64_t LB, uint64_t HB) { | |
| Wave(UINT NumWaves, uint64_t LB, uint64_t HB) { |
| uint64_t LowBits; | ||
| uint64_t HighBits; | ||
|
|
||
| Wave(CONST UINT NumWaves, uint64_t LB, uint64_t HB) { |
There was a problem hiding this comment.
I think you only ever pass LB = HB = ~0 or LB = HB = 0 to this. There may be a way to simplify to have a single value passed in?
| template <typename T> struct Op<OpType::WaveMatch, T, 1> : StrictValidation {}; | ||
|
|
||
| // Helper struct to build the expected result for WaveMatch tests. | ||
| struct Wave { |
There was a problem hiding this comment.
This probably needs a longer name. WaveMatchResultBuilder?
| uint64_t LowWaveMask; | ||
| uint64_t HighWaveMask; | ||
|
|
||
| uint64_t LowBits; | ||
| uint64_t HighBits; |
There was a problem hiding this comment.
We should aim to make these private.
What if there was function that looked like this?
void SetExpected(std::vector<UINT>::iterator It)
That would write the values into the 4 elements of the vector?
There was a problem hiding this comment.
I'm not sure taking an iterator is a great suggestion. Maybe void SetExpected(UINT* Dest) might be a better starting point.
alsepkow
left a comment
alsepkow
left a comment
There was a problem hiding this comment.
Made some comments. It looks like we need a few fixes on this iteration still.
|
|
||
| public: | ||
| WaveMatchResultBuilder(UINT NumWaves) : LowBits(0), HighBits(0) { | ||
| const UINT LowWaves = std::min(64U, NumWaves); |
There was a problem hiding this comment.
nit: Take it or leave it. But I would consider defensively adding a 'VERIFY_IS_TRUE(NumWaves <= 128)' just to help identify this path as needing an update if/when we ever increase the max size.
That or you could also do a std::max for the HighWaves logic.
Damyan also mentioned its highly unlikely this number changes. So, it's probably overkill.
| const UINT HighWaves = NumWaves - LowWaves; | ||
| LowWaveMask = (LowWaves < 64) ? (1ULL << LowWaves) - 1 : ~0ULL; | ||
| HighWaveMask = (HighWaves < 64) ? (1ULL << HighWaves) - 1 : ~0ULL; | ||
| LowBits &= LowWaveMask; |
There was a problem hiding this comment.
The assignment to LowBits and HighBits seems redundant? They're initialized to zero so the result of these operations will still always just be 0?
| private: | ||
| uint64_t LowWaveMask; | ||
| uint64_t HighWaveMask; | ||
| uint64_t LowBits; |
There was a problem hiding this comment.
I think LowBits/HighBits are actually intended to represent the currently active lanes for a given group?
Suggest updating the names to something like 'ActiveLanesLow' and 'ActiveLanesHigh' to better reflect that.
| D3D12_FEATURE_D3D12_OPTIONS1, &WaveOpts, sizeof(WaveOpts))); | ||
|
|
||
| WaveSize = WaveOpts.WaveLaneCountMin; | ||
| WaveSize = 128; // WaveOpts.WaveLaneCountMin; |
There was a problem hiding this comment.
I don't think you intended to leave this commented out
| WaveMatchResultBuilder(UINT NumWaves) : LowBits(0), HighBits(0) { | ||
| const UINT LowWaves = std::min(64U, NumWaves); | ||
| const UINT HighWaves = NumWaves - LowWaves; | ||
| LowWaveMask = (LowWaves < 64) ? (1ULL << LowWaves) - 1 : ~0ULL; |
There was a problem hiding this comment.
I think you could help improve readability of this code (not that its bad) by using inline helpers or static constexp helpers for some of the bit math instead.
LowWaveMask = lowNBitsSet(LowWaves);
'~0ULL' is probably fine. It's simple and common.
| private: | ||
| uint64_t LowWaveMask; | ||
| uint64_t HighWaveMask; | ||
| uint64_t LowBits; |
There was a problem hiding this comment.
Is there a reason you opted for two uint64 values instead of 4 uints or one 128-bit uint?
|
|
||
| const UINT LowWaves = std::min(64U, WaveSize); | ||
| const UINT HighWaves = WaveSize - LowWaves; | ||
| const UINT MidBit = WaveSize / 2; |
There was a problem hiding this comment.
Naming on this should be 'MidLaneID' and 'LastLaneID' respectively.
| const uint64_t LowExpected = ~1ULL & LowWaveMask; | ||
| const uint64_t HighExpected = ~0ULL & HighWaveMask; | ||
| if (I == 0 || MidBit == I || LastBit == I) { | ||
| WaveMatchResultBuilder ChangedLanes(WaveSize); |
There was a problem hiding this comment.
What were you intending to do with ChangedLanes? It's declared as a local in this for loop and not used outside of it.
4 participants
This patch modifies the Wave Match test to test modifications in different lanes and vector
indexes. This is achieved by forcing lanes
0,WAVE_SIZE/2andWAVE_SIZE -1, to modifythe vector at indexes
0,WAVE_SIZE/2orWAVE_SIZE -1, respectively.