Skip to content

Comments

NDRange3D refactor#330

Open
johnny-keker wants to merge 3 commits intomasterfrom
ndrange-refactor
Open

NDRange3D refactor#330
johnny-keker wants to merge 3 commits intomasterfrom
ndrange-refactor

Conversation

@johnny-keker
Copy link
Contributor

@johnny-keker johnny-keker commented Sep 10, 2022

This PR provides refactoring for NDRange3D feature.

Changes:

  • Removed NDRange3D from dispatch parameters
    Looks like we did not had a good reason for it to be there, as the only real use of that was caused by setting DimY and DimZ to 0 in BreakStateDispatchParameters instead of 1, which is incorrect in the first place and also refactored.
  • That means that calculation of GroupSize and NGroups no longer depends on the state of NDRange3D as 1-dimentional indexing is equivalent to 3-dimentional when DimY and DimZ are set to 1's.
  • Upper bound of the X group coordinate control now properly handles 1D indexation.
  • As a pleasant bonus - now 3D and 1D group coordinates are converted vice versa when user toggles 3D NDRange (see attached gif).

group_coords

RaisePropertyChanged(nameof(MaximumX));
if (!_projectOptions.VisualizerOptions.NDRange3D)
{
RaisePropertyChanged(nameof(MaximumX));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't remember why this line (RaisePropertyChanged(...)) was in the original code. It's not clear what it does and why it is needed, could you check what happens when you remove it?
  2. If there's a reason for having this call before setting X, Y, Z with NDRange disabled and after setting X, Y, Z with NDRange enabled, then this should be reflected in code comments, otherwise the calling order should be identical (and this line can be moved outside of if-else).

Copy link
Contributor

@Slimakanzer Slimakanzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the problem is much more complicated than we thought. We need to carefully review all lines of code that use VisualizerOptions.NDRange3D. In the following example, it's not clear why 1D groupIdx is used even with 3D Range.

https://github.com/vsrad/radeon-asm-tools/blob/master/VSRAD.Package/DebugVisualizer/GroupIndexSelector.cs#L123-L140


// OneWay bindings in XAML do not work on these properties for some reason, hence the empty setters
public uint MaximumX { get => _projectOptions.VisualizerOptions.NDRange3D ? DimX - 1 : uint.MaxValue; set { } }
public uint MaximumX { get => _projectOptions.VisualizerOptions.NDRange3D ? DimX - 1 : DimX * DimY * DimZ - 1; set { } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we need this ternary operator? DimY == DimZ == 1 in 1D dispatch

Comment on lines +84 to +95
X = X + Y * DimX + Z * DimX * DimY;
Y = 1;
Z = 1;
}
else
{
var x = X;
X = X % DimX;
Y = x % (DimX * DimY) / DimZ;
Z = x / DimX / DimY;
RaisePropertyChanged(nameof(MaximumX));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear why we need different code paths for 1D and 3D. I assume the logic should be the same for both 1D and 3D.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk of code is actually responsible for conversion 1D group index to 3D, and vice versa. So naturally, if we turned on 1D indexation, we need to do 3D->1D conversion, in other case - 1D->3D

Comment on lines +33 to +34
DimY = gridY / GroupSizeY;
DimZ = gridZ / GroupSizeZ;
Copy link
Contributor

@Slimakanzer Slimakanzer Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't remember why Math.Max is needed, but I would change DimX = gridX / GroupSizeX to DimX = GridSizeX / GroupSizeX. Same for DimY, DimZ.
  2. Is there any reason for having Math.Max(1, ...)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding Math.Max, I suspect it's used in the constructor to enforce the invariant that SizeX/Y/Z >= 1. Which is not that bad of an idea, considering that we assume it holds true in other parts of the code. I wonder, though, if it would be better to throw an exception instead of silently swallowing invalid values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants