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

8276905: Use appropriate macosx_version_minimum value while compiling metal shaders #6346

Closed
wants to merge 4 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -845,7 +845,9 @@ ifeq ($(call isTargetOs, macosx), true)
DEPS := $(SHADERS_SRC), \
OUTPUT_FILE := $(SHADERS_AIR), \
SUPPORT_DIR := $(SHADERS_SUPPORT_DIR), \
COMMAND := $(METAL) -c -std=osx-metal2.0 -o $(SHADERS_AIR) $(SHADERS_SRC), \
COMMAND := $(METAL) -c -std=osx-metal2.0 \
-mmacosx-version-min=$(MACOSX_VERSION_MIN) \
-o $(SHADERS_AIR) $(SHADERS_SRC), \
Copy link
Contributor

@prrace prrace Nov 12, 2021

Choose a reason for hiding this comment

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

No .. we decided metal requires macos 10.14 as a minimum. In fact it won't run (by policy) on earlier releases so settting it to what the broader JDK defines as a minimum is not appropriate right now.
And I've no idea what restrictions we'd be placing on metal saying it can only use 10.12 features.
Nor do I know what the xcode 12.4 used to build JDK 17 defaults to and how much a change to either 10.12 or 10.14 might require in re-testing.

So -
we should use 10.14
what's the actual impact of that on a current build using xcode 12.4 - does it change anything we use ?

Copy link
Contributor

@prrace prrace Nov 12, 2021

Choose a reason for hiding this comment

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

bear in mind "impact" might mean metal can't use some new faster code, not just runtime errors

Copy link
Member Author

@jayathirthrao jayathirthrao Nov 15, 2021

Choose a reason for hiding this comment

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

Hi Phil,

Thanks for the review. I also had doubts using 10.12. And yes as you mentioned we dont use Metal pipeline for versions < macOS10.14.

We dont use any Metal API which is specific to >macOS10.14(We had such usage when Lanai was under development but they were removed subsequently). So i dont see any performance impact of making xcrun to use 10.14 as minimum version.

Thanks,
Jay

))

$(eval $(call SetupExecute, metallib_shaders, \