Execution Tests: Long Vectors - Finish the basic WaveOp tests #7913
Conversation
…r floating point ops for now.
| std::numeric_limits<int16_t>::max()); | ||
| INPUT_SET(InputSet::SelectCond, 0, 1); | ||
| INPUT_SET(InputSet::AllOnes, 1); | ||
| INPUT_SET(InputSet::AllScalarOnes, 1); |
There was a problem hiding this comment.
Isn't the "scalar" part implicit in all of these input sets? What's special about this one that means we need to call them out?
There was a problem hiding this comment.
Yeah, I'll revert this. For context I changed this name when I was also adding an input set that had all bits set to 1. I was trying to differentiate between them.
| // ExpectedBuilder - specializations are expected to have buildExpectedData | ||
| // member functions. | ||
| template <OpType OP, typename T> struct ExpectedBuilder; | ||
| template <OpType OP, typename T> struct WaveOpExpectedBuilder; |
There was a problem hiding this comment.
I can't find where this is used?
There was a problem hiding this comment.
The specializations for WaveActiveAllEqual, WaveReadLaneAt, and WaveReadLaneFirst need it.
Although, they aren't making use of WaveSize argument to the WaveOpExpectedBuilder::buildExpected call. So, they could use the base one (ExpectedBuilder). But then I'd need to do something to call the right buildExpected in dispatchWaveTest.
There was a problem hiding this comment.
Ah, it looks like WaveOpExpectedBuilder was added in an earlier change, so the call to it doesn't appear in the diffs for this one.
There was a problem hiding this comment.
Yeah, it just didn't need the forward declaration when it was added.
There was a problem hiding this comment.
I missed this in the previous change - I don't quite get why we need ExpectedBuilder as well as WaveOpExpectedBuilder?
There was a problem hiding this comment.
There were a few reasons initially. Now the only thing it's getting is the WaveCount argument for buildExpected.
There was a problem hiding this comment.
The specializations don't need to match the argument structure of any of the other specializations.
There was a problem hiding this comment.
One of the other things I was going for there was to have the name be clear that it was for use with wave ops. But I guess that doesn't really matter. There's no reason another test couldn't use a wave size if it wanted to.
I've moved it back into the ExpectedBuilder.
Still debating if putting 'waveOp' in the name would make sense. It is heavily implied without it...
3 participants
This PR is the third and final PR to resolve #7472.
Test cases validated against a private build of WARP with bug fixes.