Skip to content

Conversation

@pauzinl
Copy link

@pauzinl pauzinl commented Sep 16, 2021

There could be cases when return type of intrinsic is greater than 64 moves return value into argument list as sret parameter.

TODO: currently sret parameter attribute is being lost during reverse translation for the mentioned case, need to fix it.

@pauzinl
Copy link
Author

pauzinl commented Sep 16, 2021

Hi, @MrSidims, @mlychkov. Could you please review this fix?

@MrSidims
Copy link
Contributor

MrSidims commented Sep 16, 2021

Title comment: sincos_fixed() - is a SYCL math API. Here you bring a fix for a bunch of fixed point integer SPIR-V instructions, so the name should express it.
Please also add a description of the bug and a test.

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

I believe, we should face the same issue for ArbitraryFloat instructions as well. I suggest to fix it simultaneously. Also some code from ArbitraryFloat can be adopted for fixed integer point instructions, but better to sync with @mlychkov about it.

@pauzinl pauzinl changed the title Fix for sincos_fixed() fail with certain inputs during translation Fix for a bunch of fixed point integer SPIR-V instructions Sep 17, 2021
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Functionally and test-wise LGTM, thanks!
I just have some re-wording suggestion, I'll post next.

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Suggesting in the code comment is also applicable to the PR comment.

Copy link
Contributor

@mlychkov mlychkov left a comment

Choose a reason for hiding this comment

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

Thank you.

@MrSidims MrSidims merged commit a75a62e into KhronosGroup:master Sep 20, 2021
MrSidims pushed a commit that referenced this pull request Oct 14, 2021
… bits (#1244)

This if fix for #1213
This patch adds Store instructions in cases when return value is wider than 64 bits
Quetzonarch pushed a commit to Quetzonarch/SPIRV-LLVM-Translator that referenced this pull request Jul 13, 2022
…oup#1213)

If the return type of an instruction is wider than 64-bit, then this
instruction will return via 'sret' argument added into the arguments
list. Here we reverse this, removing 'sret' argument and restoring
the original return type.

TODO: currently 'sret' parameter attribute is being lost during reverse
translation for the mentioned case, need to fix it.
Quetzonarch pushed a commit to Quetzonarch/SPIRV-LLVM-Translator that referenced this pull request Jul 13, 2022
… bits (KhronosGroup#1244)

This if fix for KhronosGroup#1213
This patch adds Store instructions in cases when return value is wider than 64 bits
vmaksimo pushed a commit to vmaksimo/SPIRV-LLVM-Translator that referenced this pull request Sep 1, 2022
…oup#1213) (KhronosGroup#15)

If the return type of an instruction is wider than 64-bit, then this
instruction will return via 'sret' argument added into the arguments
list. Here we reverse this, removing 'sret' argument and restoring
the original return type.

TODO: currently 'sret' parameter attribute is being lost during reverse
translation for the mentioned case, need to fix it.
vmaksimo pushed a commit to vmaksimo/SPIRV-LLVM-Translator that referenced this pull request Sep 1, 2022
…nstructions wi.. '"master"' -> '"xmain-web"' (26 commits)

  CONFLICT (content): Merge conflict in test/transcoding/capability-arbitrary-precision-fixed-point-numbers.ll
  CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVWriter.cpp

  commit 7904ea9
  Author: Leonid Pauzin <[email protected]>
  Date:   Thu Oct 14 19:53:45 2021 +0300

      Store result of fixed point integer SPIR-V instructions wider than 64 bits (KhronosGroup#1244)

      This if fix for KhronosGroup#1213
      This patch adds Store instructions in cases when return value is wider than 64 bits
vmaksimo pushed a commit to vmaksimo/SPIRV-LLVM-Translator that referenced this pull request Sep 1, 2022
…er SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits) (KhronosGroup#36)

* Resolve of merge (conflict) 7904ea9 Store result of fixed point integer SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits)

  CONFLICT (content): Merge conflict in test/transcoding/capability-arbitrary-precision-fixed-point-numbers.ll
  CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVWriter.cpp

  commit 7904ea9
  Author: Leonid Pauzin <[email protected]>
  Date:   Thu Oct 14 19:53:45 2021 +0300

      Store result of fixed point integer SPIR-V instructions wider than 64 bits (KhronosGroup#1244)

      This if fix for KhronosGroup#1213
      This patch adds Store instructions in cases when return value is wider than 64 bits

* Resolve of merge (conflict) 7904ea9 Store result of fixed point integer SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits)

  CONFLICT (content): Merge conflict in test/transcoding/capability-arbitrary-precision-fixed-point-numbers.ll
  CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVWriter.cpp

  commit 7904ea9
  Author: Leonid Pauzin <[email protected]>
  Date:   Thu Oct 14 19:53:45 2021 +0300

      Store result of fixed point integer SPIR-V instructions wider than 64 bits (KhronosGroup#1244)

      This if fix for KhronosGroup#1213
      This patch adds Store instructions in cases when return value is wider than 64 bits

Co-authored-by: iclsrc <[email protected]>
vmaksimo pushed a commit to vmaksimo/SPIRV-LLVM-Translator that referenced this pull request Sep 1, 2022
…int integer SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits) (KhronosGroup#36)

* Resolve of merge (conflict) 7904ea9 Store result of fixed point integer SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits)

  CONFLICT (content): Merge conflict in test/transcoding/capability-arbitrary-precision-fixed-point-numbers.ll
  CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVWriter.cpp

  commit 7904ea9
  Author: Leonid Pauzin <[email protected]>
  Date:   Thu Oct 14 19:53:45 2021 +0300

      Store result of fixed point integer SPIR-V instructions wider than 64 bits (KhronosGroup#1244)

      This if fix for KhronosGroup#1213
      This patch adds Store instructions in cases when return value is wider than 64 bits

* Resolve of merge (conflict) 7904ea9 Store result of fixed point integer SPIR-V instructions wi.. '"master"' -> '"xmain-web"' (26 commits)

  CONFLICT (content): Merge conflict in test/transcoding/capability-arbitrary-precision-fixed-point-numbers.ll
  CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVWriter.cpp

  commit 7904ea9
  Author: Leonid Pauzin <[email protected]>
  Date:   Thu Oct 14 19:53:45 2021 +0300

      Store result of fixed point integer SPIR-V instructions wider than 64 bits (KhronosGroup#1244)

      This if fix for KhronosGroup#1213
      This patch adds Store instructions in cases when return value is wider than 64 bits

Co-authored-by: iclsrc <[email protected]>
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.

3 participants