Skip to content
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.

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

HCC RT Header needs to trigger re-build #358

Merged
merged 6 commits into from
Jun 15, 2017

Conversation

aaronenyeshi
Copy link
Contributor

This resolves JIRA Ticket SWDEV-123229 - HCC RT header changes don't trigger a re-build.

There was an issue with HCC RT Headers not triggering a re-build when running make. This was due to dependencies not being properly set and checked for headers in hcc/include.

SWDEV-123229 - HCC RT header changes don't trigger a re-build.

When calling "make" to rebuild hcc after modifying the HCC RT headers, the build is not able to detect the change and to rebuild the .cpp files that include the headers being modified.  As a result, developers may be getting an hcc built from stale objects.
@aaronenyeshi
Copy link
Contributor Author

I have tested this on unit tests, and there are no new testcase failures with this. With this fix, developers can modify headers such as kalmar_runtime.h and run make which will be triggered from header file changes.

Please let me know if I should also make this change for files include/coordinate, include/array_view, and subdirectory include/experimental/.

@whchung
Copy link
Collaborator

whchung commented Jun 14, 2017

@aaronenyeshi thanks for this PR! please help amend it for other headers within include directory. Thanks.

hc_norm_unorm.inl
hc_short_vector.inl
kalmar_short_vectors.inl)

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a way to automatically generate these lists instead of constructing them manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look

@scchan scchan requested a review from pfultz2 June 14, 2017 20:36
Generate the dependency without having to specify each header files explicitly.
Copy link
Contributor Author

@aaronenyeshi aaronenyeshi left a comment

Choose a reason for hiding this comment

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

Just added another commit to generate the dependency without having to specify each header files explicitly, using regular expressions on the file names.

I still need to add another commit for other header files in the include directory.

I've added the other header files in the include sub-directories. I've also created a target specific for pstl-headers.
@aaronenyeshi
Copy link
Contributor Author

Okay, I've made it generate lists automatically for the header files.
In addition, I've included the other headers inside include .
Also, now there is a target for hcc-headers, and pstl-headers.

These commits does not produce any new testcase failures. Please take a look. Thanks!

Copy link
Collaborator

@whchung whchung left a comment

Choose a reason for hiding this comment

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

LGTM. @scchan and @pfultz2 ?

@scchan
Copy link
Collaborator

scchan commented Jun 15, 2017

looks like the build failed to generate the .deb package.
@aaronenyeshi could you do a "make package" and see if you could reproduce it?

Fix Jenkin build errors. Seems it dropped the impl/ sub-directory name in cmake_install.cmake.
@aaronenyeshi
Copy link
Contributor Author

aaronenyeshi commented Jun 15, 2017

@scchan Yes, I was able to find the bug, and fix it. Seems it dropped the sub-directory impl/ string in the cmake-install.cmake built file.

# Create target for hcc-headers and set dependencies
add_custom_target(hcc-headers ALL DEPENDS ${out_files})
add_dependencies(world hcc-headers)
set_target_properties(hcc-headers PROPERTIES FOLDER "HCC Misc")
Copy link
Contributor Author

@aaronenyeshi aaronenyeshi Jun 15, 2017

Choose a reason for hiding this comment

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

This line (35) might not be useful, as its meant for MS Visual Studio UI navigation. Should I remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aaronenyeshi thanks for spotting this. let's remove it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I think it is ready to be merged.

Not using Visual Studio UI Navigation here.
@whchung whchung merged commit fb80207 into ROCm:clang_tot_upgrade Jun 15, 2017
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