This is an archive of the discontinued LLVM Phabricator instance.

Prevent adding module flag - amdgpu_hostcall multiple times.
ClosedPublic

Authored by pvellien on Dec 23 2021, 3:03 AM.

Details

Summary

HIP program with printf call fails to compile with -fsanitize=address option, because of appending module flag - amdgpu_hostcall twice, one for printf and one for sanitize option. This patch fixes that issue.

Diff Detail

Event Timeline

pvellien created this revision.Dec 23 2021, 3:03 AM
pvellien requested review of this revision.Dec 23 2021, 3:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 3:03 AM
yaxunl accepted this revision.Dec 23 2021, 6:40 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Dec 23 2021, 6:40 AM
lebedev.ri requested changes to this revision.Dec 23 2021, 6:43 AM
lebedev.ri added a subscriber: lebedev.ri.

test coverage?

This revision now requires changes to proceed.Dec 23 2021, 6:43 AM

The testcases related to this patch are already added via this patch https://reviews.llvm.org/D112820.

The testcases related to this patch are already added via this patch https://reviews.llvm.org/D112820.

If this patch does not result in test changes, then there is no test coverage.

@yaxunl It would be very much helpful to know how to write test coverage for this particular patch? thanks

yaxunl added a comment.EditedJan 4 2022, 7:35 AM

@yaxunl It would be very much helpful to know how to write test coverage for this particular patch? thanks

there is a lit test amdgpu-asan.cu. You can add a call of printf to that test and make sure it compiles.

pvellien updated this revision to Diff 398519.Jan 9 2022, 11:34 PM

@lebedev.ri updated with test-cases.

I think it will be cleaner to keep the original amdgpu-asan.cu unchanged whereas add amdgpu-asan-printf.cu which tests asan with printf.

pvellien updated this revision to Diff 400750.Jan 18 2022, 12:23 AM

Removed amdgpu-asan-noprintf.cu and added amdgpu-asan-printf.cu testcase.

yaxunl accepted this revision.Jan 18 2022, 6:21 AM

LGTM. Thanks.

LGTM. Thanks.

Could you please commit on my behalf? I don't have a commit access to llvm trunk

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2022, 9:52 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.