Skip to content

Conversation

@Soleod
Copy link
Collaborator

@Soleod Soleod commented May 13, 2025

For validator to work you need spirv-val executable in system path. It is part of https://github.com/KhronosGroup/SPIRV-Tools. If you want to disable validation completely then you need to set env variable CYFRA_DISABLE_SPIRV_VALIDATION to on.

@Soleod Soleod requested a review from szymon-rd May 13, 2025 20:05
@Soleod Soleod self-assigned this May 13, 2025
@Soleod Soleod added the enhancement New feature or request label May 13, 2025
import scala.language.postfixOps

class MVPContext extends GContext {
implicit val ec: ExecutionContextExecutor = ExecutionContext.fromExecutor(Executors.newFixedThreadPool(16))
Copy link
Member

Choose a reason for hiding this comment

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

Let's add parameter to MVPContext that defaults to true instead of env

dumpSpvToFile(shaderCode, "program.spv")

if (SpirvValidator.isSpirvValidationEnabled)
SpirvValidator.validateSpirv(Paths.get("program.spv"))
Copy link
Member

Choose a reason for hiding this comment

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

We need to dump to tmp file instead


if (SpirvValidator.isSpirvValidationEnabled)
SpirvValidator.validateSpirv(Paths.get("program.spv"))
// SpirvValidator.corruptSpirvFile(Paths.get("program.spv"), Paths.get("corrupted_program.spv"))
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove those comments

case None => true
}

def corruptSpirvFile(originalSpirvFilePath: Path, corruptedSpirvFilePath: Path): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this before merging

@szymon-rd
Copy link
Member

Let's also add a test for this


exitCode match {
case 0 =>
println("SPIRV-Tools Validator succeeded!")
Copy link
Member

Choose a reason for hiding this comment

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

Now we have a Logger in Utility. Let's do logger.debug instead of println here

}

case None =>
println("Validator executable not found.")
Copy link
Member

Choose a reason for hiding this comment

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

I think no need to print here as we do it already when finding

} finally {
Try(Files.deleteIfExists(tmpSpirvPath)).recover {
case e: Exception =>
println(s"Failed to delete tmp spv file: ${e.getMessage}")
Copy link
Member

Choose a reason for hiding this comment

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

Here logger.error

exitCode match
case 0 => Some(validatorName)
case _ =>
println(s"Warning: SPIRV-Tools Validator wasn't found in system path.\n${stderr.toString()}")
Copy link
Member

Choose a reason for hiding this comment

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

Here logger.warn

H <: Value : Tag : FromExpr,
R <: Value : Tag : FromExpr
](function: GFunction[G, H, R]): ComputePipeline = {
](function: GFunction[G, H, R], enableSpirvValidation: Boolean = true): ComputePipeline = {
Copy link
Member

Choose a reason for hiding this comment

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

We should move that param to GContext as discussed

@Soleod Soleod changed the title Initial SpirvValidator implementation Initial SpirvTools integration Jun 1, 2025
val processIO = new ProcessIO(
in => {
try {
val buf = new Array[Byte](1024)
Copy link
Member

Choose a reason for hiding this comment

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

I think this code is repeated n times over the files, let's maybe move it to some function

arr
}

val inputStream = new ByteArrayInputStream(inputBytes)
Copy link
Member

Choose a reason for hiding this comment

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

I think that overall we do the same thing for all the tools, but with just a different binary, so that could also be merged if I'm not wrong

import java.nio.ByteBuffer
import scala.sys.process.*

class SpirvDisassemblerError(msg: String) extends RuntimeException(msg)
Copy link
Member

Choose a reason for hiding this comment

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

It should be a case class and it would be better to put it in companion object (it's a convention to limit top-level definitions that are named differently than the file)

new ComputePipeline(shader, vkContext)
}

def time[R](block: => R): R = {
Copy link
Member

Choose a reason for hiding this comment

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

I think we have this somewhere, like in Util object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't able to find it


test("Render julia set optimized"):
given GContext = new GContext(spirvOptimization = Enable(O))
val dim = 4096
Copy link
Member

Choose a reason for hiding this comment

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

and here code is repeated, we can have just one function that requires GContext and is called from both those tests

val processIO = new ProcessIO(
in => {
try {
val buf = new Array[Byte](1024)
Copy link
Member

Choose a reason for hiding this comment

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

This is still repeated 3 times I think

}
}

override protected def createError(message: String): SpirvDisassemblerError =
Copy link
Member

Choose a reason for hiding this comment

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

let's jsut get rid of all that and just do some SpirvToolException

@Soleod Soleod marked this pull request as ready for review June 15, 2025 17:06
Soleod added 6 commits June 16, 2025 19:05
# Conflicts:
#	cyfra-e2e-test/src/test/scala/io/computenode/cyfra/e2e/juliaset/JuliaSet.scala
# Conflicts:
#	cyfra-e2e-test/src/test/resources/io/computenode/cyfra/e2e/juliaset/julia.png
#	cyfra-e2e-test/src/test/resources/io/computenode/cyfra/juliaset/julia.png
#	cyfra-e2e-test/src/test/resources/julia.png
#	cyfra-e2e-test/src/test/scala/io/computenode/cyfra/e2e/juliaset/JuliaSet.scala
#	cyfra-examples/src/main/scala/io/computenode/samples/cyfra/foton/AnimatedJulia.scala
#	cyfra-foton/src/main/scala/io/computenode/cyfra/foton/animation/AnimatedFunctionRenderer.scala
#	cyfra-runtime/src/main/scala/io/computenode/cyfra/runtime/GContext.scala
@Soleod Soleod merged commit 41267ce into main Jun 16, 2025
1 check passed
@Soleod Soleod deleted the SPIRV_INTEGRATION branch June 16, 2025 19:14
MarconZet pushed a commit that referenced this pull request Oct 20, 2025
SpirvTools are now integrated into the flow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants