Fix SPV_INTEL_runtime_aligned implementation part 1 #1796

MrSidims · 2022-12-28T14:28:39Z

It was implemented via new decoration, which is not correct. Instead it should be Function Parameter Attribute decoration.

In this commit starts fixing this in step-by-step manner.

This step only adds reverse translation to be reused later by SPIR-V consumers.

Link to spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_runtime_aligned.asciidoc

Signed-off-by: Sidorov, Dmitry [email protected]

It was implemented via new decoration, which is not correct. Instead it should be Function Parameter Attribute decoration. In this commit starts fixing this in step-by-step manner. Signed-off-by: Sidorov, Dmitry <[email protected]>

MrSidims · 2023-02-09T12:43:19Z

@broxigarchen may I ask you to take a look?

MrSidims · 2023-03-15T15:02:27Z

@broxigarchen @tiwaria1 @asudarsa @vmaksimo @jgstarIntel @maksimsab please take a look

vmaksimo

Only from the code I understood that somehow we want to save the non-conformant implementation for now. Please update the description about what this step do. Also the link to the spec could be useful.

Am I right that test/transcoding/SPV_INTEL_runtime_aligned/RuntimeAligned.ll is not changed for now because we preserve the old implementation?

vmaksimo · 2023-03-15T15:25:05Z

lib/SPIRV/SPIRVReader.cpp

    mapValue(BA, &(*I));
    setName(&(*I), BA);
    BA->foreachAttr([&](SPIRVFuncParamAttrKind Kind) {
+      // Skip this function parameter attribute as it will translated among


Just to clarify, why wasn't it skipped before?

Because it wasn't a function parameter attribute (since it was implemented incorrectly)

tiwaria1 · 2023-03-15T15:50:11Z

Viktoria this is the spec doc: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_runtime_aligned.asciidoc

Yes it should be function parameter attribute as per the spec

tiwaria1 · 2023-03-15T16:00:22Z

Hey Dmitry, what did this look like in IR after reverse-translation before this change? What will the IR be after this change? (I am guessing kernel parameter attribute?)

MrSidims · 2023-03-15T20:08:29Z

Hey Dmitry, what did this look like in IR after reverse-translation before this change? What will the IR be after this change? (I am guessing kernel parameter attribute?)

@tiwaria1 It remains to be the same - a function metadata looking like:
!kernel_arg_runtime_aligned !N
!N = !{i1 true, i1 false, i1 true, i1 false, i1 false}
(see both CHECK-LLVM in the test in this PR and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/extensions/INTEL/SPV_INTEL_runtime_aligned/RuntimeAligned.ll for reference

MrSidims · 2023-03-15T20:10:40Z

Only from the code I understood that somehow we want to save the non-conformant implementation for now. Please update the description about what this step do. Also the link to the spec could be useful.

Am I right that test/transcoding/SPV_INTEL_runtime_aligned/RuntimeAligned.ll is not changed for now because we preserve the old implementation?

Added in the description. Yeap, the test is unchanged intentionally

tiwaria1 · 2023-03-15T21:12:37Z

Hey Dmitry, what did this look like in IR after reverse-translation before this change? What will the IR be after this change? (I am guessing kernel parameter attribute?)

@tiwaria1 It remains to be the same - a function metadata looking like: !kernel_arg_runtime_aligned !N !N = !{i1 true, i1 false, i1 true, i1 false, i1 false} (see both CHECK-LLVM in the test in this PR and https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/extensions/INTEL/SPV_INTEL_runtime_aligned/RuntimeAligned.ll for reference

Thanks, I asked to understand if FPGA backend would need to be updated after this fix to handle IR changes. Looks like it will not be needed.

broxigarchen · 2023-03-15T22:23:22Z

lib/SPIRV/SPIRVReader.cpp

  BF->foreachArgument([&](SPIRVFunctionParameter *Arg) {
-    if (Arg->getType()->isTypePointer() &&
+    if (Arg->hasAttr(FunctionParameterAttributeRuntimeAlignedINTEL) ||
        Arg->hasDecorate(internal::DecorationRuntimeAlignedINTEL)) {


we finally will remove this line
"Arg->hasDecorate(internal::DecorationRuntimeAlignedINTEL)"
right?

Right, I believe that is the intention - but only after consumers are ready to work with the correct implementation.

broxigarchen · 2023-03-15T22:26:33Z

Sorry I guess your previous email get buried in my mail box and I just saw it now.

PR looks good to me. I guess the SPIRV consumer might need to merge the patch later after it's finalized. Just curious why we need to do this step by step?

asudarsa · 2023-03-16T00:24:07Z

lib/SPIRV/SPIRVReader.cpp

        Arg->hasDecorate(internal::DecorationRuntimeAlignedINTEL)) {
-      DecorationFound = true;
+      RuntimeAlignedFound = true;
      ValueVec.push_back(ForeachFnArg(Arg));


ForeachFnArg seems unnecessary here. We can just create a ConstantInt with 1 and push it here.

It actually does exactly what you're mentioned, see https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/1796/files#diff-7990d99acb2f8325d245dfecdab758eae5aebae2b54c04fd8e906212eb80ae10R4472-R4473

asudarsa · 2023-03-16T00:28:19Z

lib/SPIRV/libSPIRV/SPIRVIsValidEnum.h

  case FunctionParameterAttributeNoCapture:
  case FunctionParameterAttributeNoWrite:
  case FunctionParameterAttributeNoReadWrite:
+  case FunctionParameterAttributeRuntimeAlignedINTEL:


I suppose this change will be useful when SPIRVWriter.cpp is updated.

asudarsa · 2023-03-16T00:30:57Z

lib/SPIRV/SPIRVReader.cpp

+      RuntimeAlignedFound = true;
      ValueVec.push_back(ForeachFnArg(Arg));
    } else {
      llvm::Metadata *DefaultNode = ConstantAsMetadata::get(


One nit: Instead of creating DefaultNode for each Argument, we can create a single static global DefaultNode and push it when required.

Guess no need to have it as a global, moved it out of lambda to have it initialized only once in the function.

asudarsa

Few nits. Are not PR blocking.

MrSidims · 2023-03-16T10:28:55Z

Just curious why we need to do this step by step?

Without consumers update such SPIR-V changes will result in a missing feature. And once it's updated - it takes time until new driver is being propagated in different CIs (and SPIR-V generator change might reach CI at this point). Also I don't have much control over every SPIR-V consumer, hence prefer to go with conservative approach anyway.

asudarsa · 2024-02-13T20:36:29Z

Closing and reopening to run tests.

asudarsa · 2024-02-13T20:38:09Z

Hi @MrSidims,

Is this change ready to be merged? Also, IIUC, the forward translation is the next and final step in this effort?

Thanks

asudarsa · 2024-06-19T01:45:16Z

test/extensions/INTEL/SPV_INTEL_runtime_aligned/RuntimeAligned.spt

@@ -0,0 +1,68 @@
+; RUN: llvm-spirv -spirv-text -r %s -o %t.bc


Not a blocker for this PR. Going forward, it will be great if we can introduce such .spt tests by generating them using the spirv-dis tool which generates a more readable file.

Thanks

asudarsa

Translator change Looks good. Thanks

Fix SPV_INTEL_runtime_aligned implementation part 1

879cb60

It was implemented via new decoration, which is not correct. Instead it should be Function Parameter Attribute decoration. In this commit starts fixing this in step-by-step manner. Signed-off-by: Sidorov, Dmitry <[email protected]>

vmaksimo reviewed Mar 15, 2023

View reviewed changes

broxigarchen reviewed Mar 15, 2023

View reviewed changes

asudarsa reviewed Mar 16, 2023

View reviewed changes

asudarsa closed this Feb 13, 2024

asudarsa reopened this Feb 13, 2024

vmaksimo added 3 commits May 23, 2024 03:03

Merge remote-tracking branch 'origin/main' into HEAD

918546c

fix test

6e81c1e

apply review suggestion

4df20c2

vmaksimo requested a review from asudarsa May 23, 2024 11:11

asudarsa reviewed Jun 19, 2024

View reviewed changes

asudarsa approved these changes Jun 19, 2024

View reviewed changes

vmaksimo requested a review from broxigarchen June 25, 2024 10:53

broxigarchen approved these changes Jun 25, 2024

View reviewed changes

MrSidims merged commit 0db0f98 into KhronosGroup:main Jun 26, 2024

vmaksimo mentioned this pull request May 12, 2025

Enabled RuntimeAlignedINTEL functionParameterAttribute. #3141

Open

		@@ -0,0 +1,68 @@
		; RUN: llvm-spirv -spirv-text -r %s -o %t.bc

Fix SPV_INTEL_runtime_aligned implementation part 1 #1796

Fix SPV_INTEL_runtime_aligned implementation part 1 #1796

Uh oh!

Conversation

MrSidims commented Dec 28, 2022 • edited by asudarsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrSidims commented Feb 9, 2023

Uh oh!

MrSidims commented Mar 15, 2023

Uh oh!

vmaksimo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiwaria1 commented Mar 15, 2023

Uh oh!

tiwaria1 commented Mar 15, 2023

Uh oh!

MrSidims commented Mar 15, 2023

Uh oh!

MrSidims commented Mar 15, 2023

Uh oh!

tiwaria1 commented Mar 15, 2023

Uh oh!

broxigarchen Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

broxigarchen commented Mar 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

MrSidims commented Mar 16, 2023

Uh oh!

asudarsa commented Feb 13, 2024

Uh oh!

asudarsa commented Feb 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MrSidims commented Dec 28, 2022 •

edited by asudarsa

Loading

broxigarchen Mar 15, 2023 •

edited

Loading