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

problems with OpCopyMemory #2769

Closed
bashbaug opened this issue Oct 19, 2024 · 1 comment · Fixed by #2779
Closed

problems with OpCopyMemory #2769

bashbaug opened this issue Oct 19, 2024 · 1 comment · Fixed by #2779

Comments

@bashbaug
Copy link
Contributor

Reverse translation (from SPIR-V to LLVM) does not appear to be working propery for OpCopyMemory. Note that OpCopyMemorySized is working correctly, and there do not appear to be any tests for OpCopyMemory.

I am encountering two problems with the following tester (note: this is an except of a test for the SPIR-V 1.4 feature where OpCopyMemory can accept two optional memory operands):

; SPIR-V
; Version: 1.4
               OpCapability Addresses
               OpCapability Kernel
               OpMemoryModel Physical64 OpenCL
               OpEntryPoint Kernel %kernel "copymemory_test"
       %uint = OpTypeInt 32 0
       %void = OpTypeVoid
   %gptr_int = OpTypePointer CrossWorkgroup %uint
   %pptr_int = OpTypePointer Function %uint
 %kernel_sig = OpTypeFunction %void %gptr_int
    %uint_42 = OpConstant %uint 42
     %kernel = OpFunction %void None %kernel_sig
        %dst = OpFunctionParameter %gptr_int
      %entry = OpLabel
     %pvalue = OpVariable %pptr_int Function %uint_42
               OpCopyMemory %dst %pvalue
               OpReturn
               OpFunctionEnd
  1. The asserts are incorrect in SPIRV::SPIRVCopyMemory::validate, because they are checking Id, but this instruction does not set an Id. This one is pretty easy to fix - something like:

    void validate() const override {
        assert(getValueType(Target)->isTypePointer() && "Invalid Target type");
        assert(getValueType(Source)->isTypePointer() && "Invalid Source type");
        assert(!(getValueType(Target)->getPointerElementType()->isTypeVoid()) &&
               "Invalid Target pointer type");
        assert(!(getValueType(Source)->getPointerElementType()->isTypeVoid()) &&
               "Invalid Target pointer type");
        assert(getValueType(Target)->getPointerElementType() ==
                   getValueType(Source)->getPointerElementType() &&
               "Mismatched Target and Source types");
        SPIRVInstruction::validate();
    }
  2. There is no special handling for OpCopyMemory (there is for OpCopyMemorySized), so translating OpCopyMemory falls into the default transSPIRVBuiltinFromInst, which does not work properly. I suspect that OpCopyMemory should be lowered into a call to llvm.memcpy, similar to OpCopyMemorySized, but the size will be dependent on the pointed-to types vs. provided explicitly.

Once these problems are fixed we should add some tests for OpCopyMemory also.

svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 21, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769
@svenvh
Copy link
Member

svenvh commented Oct 21, 2024

Thanks for the detailed report @bashbaug!

I've gone with a minimal fix for the first issue in #2770 ; more thorough validation should be spirv-val's job.

For the second issue, I expect that we can treat OpCopyMemory pretty much like OpCopyMemorySized indeed.

svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 21, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769
svenvh added a commit that referenced this issue Oct 22, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 22, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 22, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 25, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup#2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit that referenced this issue Oct 28, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to #2769

(cherry picked from commit 9d2926d)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit to svenvh/SPIRV-LLVM-Translator that referenced this issue Oct 28, 2024
…2779)

Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes KhronosGroup#2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
svenvh added a commit that referenced this issue Oct 30, 2024
Add support for translating `OpCopyMemory` into `llvm.memcpy`.

Fixes #2769

(cherry picked from commit 8dc0349)
jsji pushed a commit to intel/llvm that referenced this issue Nov 7, 2024
The asserts should be checking the `Target` member variable; `Id` is
not used for this class.

Only fix the wrong asserts for now; proper handling and testing of
`OpCopyMemory` will be done in a followup commit.

Contributes to KhronosGroup/SPIRV-LLVM-Translator#2769

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@9d2926d27b478f0
jsji pushed a commit to intel/llvm that referenced this issue Nov 7, 2024
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 a pull request may close this issue.

2 participants