Skip to content

[SYCLomatic] Fix migration of cudaStreamDefault and cudaStreamNonBlocking#475

Merged
0x12CC merged 5 commits intooneapi-src:SYCLomaticfrom
jzc:cudastream-flags
Feb 14, 2023
Merged

[SYCLomatic] Fix migration of cudaStreamDefault and cudaStreamNonBlocking#475
0x12CC merged 5 commits intooneapi-src:SYCLomaticfrom
jzc:cudastream-flags

Conversation

@jzc
Copy link
Copy Markdown
Contributor

@jzc jzc commented Jan 4, 2023

Signed-off-by: Cai, Justin justin.cai@intel.com

@jzc jzc requested a review from a team as a code owner January 4, 2023 17:05
@jzc jzc force-pushed the cudastream-flags branch from 643f92c to 512ac87 Compare January 10, 2023 20:10
Comment thread clang/lib/DPCT/ASTTraversal.cpp Outdated
Begin, SM, DpctGlobalInfo::getContext().getLangOpts());
emplaceTransformation(new ReplaceText(Begin, Length, "0"));
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cudaStreamDefault is migrated to 0 if there has not been a replacement migrating it to &dpct::get_default_queue().

}

REGISTER_RULE(PreDefinedStreamHandleRule, PassKind::PK_Migration)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This migration rule was migrating the macros cudaStreamDefault, cudaStreamLegacy, and cudaStreamPerThread to dpct::get_default_queue(). This is correct for cudaStreamLegacy and cudaStreamPerThread, as these macros' definition expand to a value of cudaStream_t. This is incorrect for cudaStreamDefault in a edge case where you assign cudaStreamDefault to an int. (See the old macro_test.cu - the checked output is not valid C++ because you cannot assign a sycl::queue * to an int). For the migration of cudaStreamLegacy and cudaStreamPerThread, this migration behavior is capture by CudaStreamCastRule, so it can be removed.

(Note: the intended purpose of the cudaStreamDefault macro is as a flag used with cudaStreamCreateWithFlags . The name of cudaStreamDefault makes it seem like you can use it as a the default cudaStream_t, which perhaps coincidently, you can, because cudaStreamDefault expands to 0, which is the same value used to denote the default stream with the default stream semantics of the cuda api.)

@jzc jzc force-pushed the cudastream-flags branch 2 times, most recently from 23de87f to c1efdae Compare January 23, 2023 15:57
Copy link
Copy Markdown
Contributor

@LU-JOHN LU-JOHN left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread clang/lib/DPCT/ASTTraversal.cpp Outdated
@jzc jzc force-pushed the cudastream-flags branch from 0673f92 to 2b1a96e Compare February 8, 2023 18:44
@jzc jzc requested a review from ziranzha February 8, 2023 19:03
@0x12CC 0x12CC merged commit fc2959c into oneapi-src:SYCLomatic Feb 14, 2023
Copy link
Copy Markdown
Contributor

@ShengchenJ ShengchenJ left a comment

Choose a reason for hiding this comment

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

The comment is out of date
// If PredefinedStreamName is used with concatenated macro token,
// detect the previous macro expansion
please also clean it.

ShengchenJ pushed a commit to ShengchenJ/SYCLomatic that referenced this pull request Sep 27, 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 this pull request may close these issues.

5 participants