Skip to content

Conversation

@Alloyed
Copy link

@Alloyed Alloyed commented Feb 24, 2019

More details in code review comments. These changes were necessary to get microprofile working using GCC on linux, with Vulkan timers enabled.

return;

vkGetQueryPoolResults(S.pGPU->Devices[nNode], S.pGPU->QueryPool[nNode], nBegin, nCount, 8*nCount, &S.pGPU->nResults[nBegin], 8, VK_QUERY_RESULT_64_BIT|VK_QUERY_RESULT_PARTIAL_BIT );
vkGetQueryPoolResults(S.pGPU->Devices[nNode], S.pGPU->QueryPool[nNode], nBegin, nCount, 8*nCount, &S.pGPU->nResults[nBegin], 8, VK_QUERY_RESULT_64_BIT);
Copy link
Author

@Alloyed Alloyed Feb 24, 2019

Choose a reason for hiding this comment

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

https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/vkGetQueryPoolResults.html

VK_QUERY_RESULT_PARTIAL_BIT must not be used if the pool’s queryType is VK_QUERY_TYPE_TIMESTAMP.

maybe VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is the intended behavior here? Without it we skip not-ready values, with it we write in 0 for not-ready values: I don't know enough about the surrounding code to say either way.


void MicroProfileGpuShutdown()
{
VkAllocationCallbacks* noAlloc = nullptr;
Copy link
Author

Choose a reason for hiding this comment

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

Implementing cleanup. without this my particular laptop will crash on shutdown

Copy link
Owner

Choose a reason for hiding this comment

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

looks good

{
const uint32_t nLim = pTable->nLim;
uint32_t B = H % pTable->nAllocated;
uint32_t B = static_cast<uint32_t>(H) % pTable->nAllocated;
Copy link
Author

Choose a reason for hiding this comment

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

Triggers SIGFPE, because H is a uint64 and will overflow to get it to fit within the modulo operation. The cast makes the existing behavior explicit/not-error-y, but I have not checked to see what that means for the hash table generally

Copy link
Owner

Choose a reason for hiding this comment

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

-Can you move this to a seperate
-use C style casts.
-I count 3 instances of the bug, it seems you only have two of them. Can you add the last one. (one in each of Get/Set/Remove)

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.

2 participants