This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Set main as default entry.
ClosedPublic

Authored by python3kgae on May 2 2022, 12:25 AM.

Details

Summary

When there's no -E option, use main as entry function.

Diff Detail

Event Timeline

python3kgae created this revision.May 2 2022, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 12:25 AM
python3kgae requested review of this revision.May 2 2022, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 12:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith resigned from this revision.May 3 2022, 12:17 PM
Anastasia added inline comments.May 24 2022, 5:54 AM
clang/lib/Driver/ToolChains/HLSL.cpp
182 ↗(On Diff #426342)

-> If no set entry or If entry is not set explicitly

Update comments.

Anastasia added inline comments.May 26 2022, 10:48 AM
clang/test/CodeGenHLSL/entry_default.hlsl
2

Would it make sense to test the opposite i.e. if -E is passed in the command line main is not an entry point?

Add test for main is not entry.

MaskRay added inline comments.May 26 2022, 6:18 PM
clang/lib/Driver/ToolChains/HLSL.cpp
194 ↗(On Diff #432429)

Instead of letting Driver pass a default OPT_hlsl_entrypoint to CC1, you can let CC1 have a default OPT_hlsl_entrypoint.

python3kgae marked 2 inline comments as done.

Use default value of MarshallingInfoString.

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

Add new line at end of file.

MaskRay added inline comments.May 31 2022, 10:45 PM
clang/test/CodeGenHLSL/entry_default.hlsl
7

space between : and the value.

10

[[MAIN_ATTR]] and the value should be on the same line.

Cleanup test.

python3kgae marked 2 inline comments as done.Jun 1 2022, 12:18 AM

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

This test doesn't seem to correspond to the change being added as you are changing the command-line flags. You don't actually add/generate any attributes in this patch.

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

This test doesn't seem to correspond to the change being added as you are changing the command-line flags. You don't actually add/generate any attributes in this patch.

Sorry to make things confusing. I should not split default value for -E option as a separate PR :(

There's dxc_E.hlsl (https://reviews.llvm.org/D124751#change-af6Z62NjlfGb) in https://reviews.llvm.org/D124751 where the -E option is added.
dxc_E.hlsl will test -E option translated into -hlsl-entry for cc1.

There was a test for codeGen of -E option in https://reviews.llvm.org/D124752, but I removed it because it is to almost the same as https://reviews.llvm.org/D124752#change-w4NWvaT68Dhk which test codeGen for ShaderAttr.

And the entry_default.hlsl in current PR test codeGen for the default main and -E option.

I can add a separate codeGen test for -E option if that's what you thought is missing.

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

This test doesn't seem to correspond to the change being added as you are changing the command-line flags. You don't actually add/generate any attributes in this patch.

Sorry to make things confusing. I should not split default value for -E option as a separate PR :(

There's dxc_E.hlsl (https://reviews.llvm.org/D124751#change-af6Z62NjlfGb) in https://reviews.llvm.org/D124751 where the -E option is added.
dxc_E.hlsl will test -E option translated into -hlsl-entry for cc1.

There was a test for codeGen of -E option in https://reviews.llvm.org/D124752, but I removed it because it is to almost the same as https://reviews.llvm.org/D124752#change-w4NWvaT68Dhk which test codeGen for ShaderAttr.

And the entry_default.hlsl in current PR test codeGen for the default main and -E option.

I can add a separate codeGen test for -E option if that's what you thought is missing.

Yes, in this patch you are just changing the marchaling flag to -cc1 right? It would be better if you test just exactly that and then in dependent patches you can test what is related to other changes. It will make things less confusing then. ;)

bogner added inline comments.Jun 14 2022, 12:37 PM
clang/test/CodeGenHLSL/entry_default.hlsl
15

typo, should be CHECK-SAME

Fix typo in test.

python3kgae marked an inline comment as done.Jun 14 2022, 12:50 PM
python3kgae added inline comments.
clang/test/CodeGenHLSL/entry_default.hlsl
15

Good catch.
It is strange that the test will pass even with the typo.

python3kgae marked an inline comment as done.

Rebase and add unit-test.

beanz accepted this revision.Aug 18 2022, 10:36 AM
This revision is now accepted and ready to land.Aug 18 2022, 10:36 AM

Update to work around arc error when land.

This revision was landed with ongoing or failed builds.Aug 18 2022, 11:57 AM
This revision was automatically updated to reflect the committed changes.