Skip to content

[SYCL][NFC] Fix build errors revealed by self build. #3132

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 4, 2021
Merged

[SYCL][NFC] Fix build errors revealed by self build. #3132

merged 1 commit into from
Feb 4, 2021

Conversation

zahiraam
Copy link
Contributor

@zahiraam zahiraam commented Feb 1, 2021 •
edited
Loading

Clang warns about using default in a switch statement where all enumerated cases are covered. This is a patch to avoid the warning.

kbobrovs
kbobrovs reviewed Feb 2, 2021
View reviewed changes
clang/lib/CodeGen/CGSYCLRuntime.cpp Outdated
@@ -78,9 +78,9 @@ bool CGSYCLRuntime::actOnFunctionStart(const FunctionDecl &FD,
// be handled specially in the SYCL lowering pass
F.setMetadata(PFWI_MD_ID, llvm::MDNode::get(F.getContext(), {}));
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
break;
break;
default:
// fall through

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the default here will lead to the same build error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see what I am proposing as a change.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the build error? Would be good to add this info to the PR description as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a known issue about the default case of switch statement in clang.
Please see:
https://godbolt.org/z/5jvKjz
and
https://godbolt.org/z/sY6bdE
I can propose this solution.
https://godbolt.org/z/oqY8aG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more thing to confirm this change:
https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The combination of these 2 flags that will have the effect of a static. So there is really no need to add the assert as I was proposing in https://godbolt.org/z/oqY8aG.
See https://godbolt.org/z/PqE7Mf

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

So there is really no need to add the assert

I agree with this.

elizabethandrews
elizabethandrews reviewed Feb 2, 2021
View reviewed changes
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

Could you please explain why default is causing self-build issues?

@zahiraam
Copy link
Contributor Author

zahiraam commented Feb 2, 2021

Could you please explain why default is causing self-build issues?

Avoiding this error: "default label in switch which covers all enumeration values "

premanandrao
premanandrao reviewed Feb 2, 2021
View reviewed changes
clang/lib/Driver/ToolChains/Clang.cpp
@@ -8353,8 +8353,6 @@ void FileTableTform::ConstructJob(Compilation &C, const JobAction &JA,
addArgs(CmdArgs, TCArgs, {Arg});
break;
}
default:
llvm_unreachable("unknown file table transformation kind");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain this one? Are we losing the "unreachable" message for the cases not handled in the switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clang knows that the values of the switch can only be the ones listed in the cases before the default, and it gives the error that I am mentioning above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it! Sorry, I didn't realize @elizabethandrews was asking about the same label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to add, the unreachable to make sure we catch a potential wrong value of the switch then, we need to rewrite the switch statement all together. May be asserting that Tf.TheKind can only be the values listed and put that assertion before the switch statement. Let me know if you think that this is better approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the case I was concerned about is when another enumerator is added to the enum type, but this switch isn't modified. But I guess in that case, the switch statement would get a diagnostic about a missing case statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly - I was also curious and verified that

kbobrovs
kbobrovs approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

CGSYCLRuntime.cpp and ClangOffloadWrapper.cpp

mdtoguchi
mdtoguchi approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

OK For Driver - Thanks!

premanandrao
premanandrao approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes looks good to me.

@bader bader merged commit 1bdb44c into intel:sycl Feb 4, 2021
iclsrc pushed a commit that referenced this pull request May 12, 2025
@MrSidims @sys-ce-bb
Add [[maybe_unused]] to EmplaceStatus for type map (#3132)
c3120b6 
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@8011eaf0c686ecc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elizabethandrews elizabethandrews elizabethandrews left review comments

@premanandrao premanandrao premanandrao approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@AGindinson AGindinson Awaiting requested review from AGindinson

@sndmitriev sndmitriev Awaiting requested review from sndmitriev

+1 more reviewer

@kbobrovs kbobrovs kbobrovs approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zahiraam @kbobrovs @premanandrao @mdtoguchi @elizabethandrews @bader