Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spirv-val: Add SPV_ARM_core_builtins validation #4958

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

kpet
Copy link
Contributor

@kpet kpet commented Oct 3, 2022

Signed-off-by: Kevin Petit kevin.petit@arm.com
Change-Id: If1680a823aea9662d44def1ec6fe6ac334c00574

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: If1680a823aea9662d44def1ec6fe6ac334c00574
Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dneto0 dneto0 merged commit a6e6454 into KhronosGroup:master Oct 6, 2022
@kpet kpet deleted the spv-arm-core-builtins branch October 6, 2022 06:59
@jeremy-lunarg
Copy link
Contributor

Does this need a dependency bump? I'm seeing compilation errors. e.g.

/home/jeremy/Development/Vulkan/Khronos/SPIRV-Tools/source/val/validate_builtins.cpp:4228:10: error: ‘SpvBuiltInCoreIDARM’ was not declared in this scope
     case SpvBuiltInCoreIDARM:

kpet added a commit to kpet/SPIRV-Tools that referenced this pull request Oct 7, 2022
Following KhronosGroup#4958

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I0aed90dbd5705881a5f68d439f9d191d5d01c993
kpet added a commit to kpet/SPIRV-Tools that referenced this pull request Oct 7, 2022
To avoid mistakes such as the one that happened under KhronosGroup#4958.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I36ba10cffb61d5a934aa95da51ad09bfdc5c03a5
@kpet
Copy link
Contributor Author

kpet commented Oct 7, 2022

Yes, it does. I've created #4962. This change should never have gone in as is. A few thoughts on what went wrong:

  • This PR passed the CI which shouldn't have happened. I'm guessing the CI doesn't use the revisions from DEPS.
  • git-sync-deps clones the headers into external/spirv-headers and was checking out the correct revision but I also had a copy under external/SPIRV-Headers where my changes were. When an external path to the headers is not provided via SPIRV-Headers_SOURCE_DIR the build system tries to use external/SPIRV-Headers or external/spirv-headers in that order. In my local testing, I was calling git-sync-deps and then building and running tests but the headers checked out by git-sync-deps were never used.

A couple of proposed improvements:

alan-baker pushed a commit that referenced this pull request Oct 11, 2022
Following #4958

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I0aed90dbd5705881a5f68d439f9d191d5d01c993

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
kpet added a commit to kpet/SPIRV-Tools that referenced this pull request Oct 27, 2022
This should help with avoiding mistakes such as the one that happened under KhronosGroup#4958.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I922f02e25c507f3412e0e7a99f525fb617b2d426
@s-perron
Copy link
Collaborator

Fix the CI so the revisions used all come from DEPS. Does this need an issue?

We have made a conscious decision not to do this for all builds because we want to make sure everything always builds a head. DEPS is updated when doing a release, and at other adhoc times.

I'd be willing to do all CI with DEPS if we had an autoroller, to essentially always keep DEPS at head. Or we could get 1 build to build with DEPS to make sure it always works. We will consider this. Thanks.

kpet added a commit to kpet/SPIRV-Tools that referenced this pull request Jan 24, 2023
This should help with avoiding mistakes such as the one that happened under KhronosGroup#4958.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I922f02e25c507f3412e0e7a99f525fb617b2d426
s-perron pushed a commit that referenced this pull request Jan 24, 2023
#4963)

This should help with avoiding mistakes such as the one that happened under #4958.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
Change-Id: I922f02e25c507f3412e0e7a99f525fb617b2d426

Signed-off-by: Kevin Petit <kevin.petit@arm.com>
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.

4 participants