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

Fix invalid-device-fn error in cudf::strings::replace_re with multiple regex's #7336

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

davidwendt
Copy link
Contributor

Found errors when testing libcudf built with gcc-9 and nvcc 11.1.

STRINGS_TEST : StringsReplaceTests.ReplaceMultiRegexTest
MERGE_TEST : MergeStringTest/8.Merge2StringKeyColumns (with BOOL8 column types only)
PARTITIONING_TEST : RoundRobinTest/8.RoundRobin (tests with BOOL8 column types only)

The strings test cause an invalid-device-function error. The code for cudf::strings::replace_re that accepts multiple regex's was fixed to better manage the device memory holding the regex structures. The error occurred when cleaning up the temporary memory.

The other two tests failed because the iterator used for generating the expected data (in CPU code) was producing the wrong results. This may be a gcc optimization bug since adding a printf cleared the error. I was able to recode the iterator's functor so it would succeed on both gcc9 and gcc7.

@davidwendt davidwendt added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Feb 5, 2021
@davidwendt davidwendt self-assigned this Feb 5, 2021
@davidwendt davidwendt requested a review from a team as a code owner February 5, 2021 21:55
@davidwendt davidwendt changed the title Fix invalid-device-fn cudf::strings::replace_re with multiple regex's Fix invalid-device-fn error in cudf::strings::replace_re with multiple regex's Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@eb8dc88). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7336   +/-   ##
==============================================
  Coverage               ?   82.20%           
==============================================
  Files                  ?      100           
  Lines                  ?    16966           
  Branches               ?        0           
==============================================
  Hits                   ?    13947           
  Misses                 ?     3019           
  Partials               ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb8dc88...9a65618. Read the comment docs.

Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

edit: forgot to remove this comment. Github won't let me delete for some reason.

cpp/src/strings/replace/multi_re.cu Outdated Show resolved Hide resolved
cpp/src/strings/replace/multi_re.cu Show resolved Hide resolved
@davidwendt davidwendt requested a review from nvdbaranec February 8, 2021 20:29
@harrism harrism closed this Feb 9, 2021
@harrism
Copy link
Member

harrism commented Feb 9, 2021

@gpucibot merge

@ajschmidt8 ajschmidt8 reopened this Feb 10, 2021
@rapids-bot rapids-bot bot merged commit da3ab29 into rapidsai:branch-0.19 Feb 10, 2021
@davidwendt davidwendt deleted the build-gcc9 branch February 10, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants