This is an archive of the discontinued LLVM Phabricator instance.

[DX] Fix PSV resource serialization
ClosedPublic

Authored by beanz on Jul 12 2023, 6:57 PM.

Details

Summary

When writing this initially I missed including the resource stride.
This change adds the resources stride to the serialized value.

I've also extended the testing and error reporting around parsing PSV
information. This adds tests to verify that the reader produces
meaningful error messages for malformed DXContainer files, and a test
that verifies the resource stride is respected in the reader even if
the stride isn't an expected or known value (as would happen if the
format changes in the future).

This is part of #59479.

Diff Detail

Event Timeline

beanz created this revision.Jul 12 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 6:57 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
beanz requested review of this revision.Jul 12 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 6:57 PM
bogner accepted this revision.Jul 17 2023, 1:29 PM
This revision is now accepted and ready to land.Jul 17 2023, 1:29 PM
bob80905 accepted this revision.Jul 17 2023, 4:09 PM
bob80905 added a subscriber: bob80905.

LGTM

llvm/include/llvm/Object/DXContainer.h
95–96

Just curious whether this should be revisited, or why these operators aren't defined in terms of the opposite of the other.

beanz added inline comments.Jul 17 2023, 5:37 PM
llvm/include/llvm/Object/DXContainer.h
95–96

This is some funny syntax, but the != operator is implemented in terms of the == operator, which only does the pointer compare on the Current pointer.

The other way the != operator could be written would be something like:

bool operator!=(const iterator I) { return !operator==(I); }

That should be equivalent to the code currently in there because it is just calling == on two iterator objects.

This revision was landed with ongoing or failed builds.Jul 19 2023, 4:02 PM
This revision was automatically updated to reflect the committed changes.