This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtr][Inline] Use byval type instead of pointee type
ClosedPublic

Authored by aeubanks on Jul 9 2021, 9:38 AM.

Details

Reviewers
dblaikie
Group Reviewers
Restricted Project
Commits
rG33d44b762e65: [OpaquePtr][Inline] Use byval type instead of pointee type

Diff Detail

Event Timeline

aeubanks created this revision.Jul 9 2021, 9:38 AM
aeubanks requested review of this revision.Jul 9 2021, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 9:38 AM
aeubanks added a reviewer: Restricted Project.Jul 9 2021, 9:39 AM

Can this be tested with some opaque pointer IR using byval?

llvm/lib/Transforms/Utils/InlineFunction.cpp
1896

Might be reaching the point where a struct with named members would be suitable/more self-documenting?

aeubanks updated this revision to Diff 358138.Jul 12 2021, 9:05 PM

make tuple into struct
add assert

dblaikie accepted this revision.Jul 13 2021, 10:28 AM

As per ( https://reviews.llvm.org/D105710#2874527 ) I still think there's room for valuable testing at this stage in the opaque pointer work.

Partly I think it's a matter of changing the approach from source based to test based - eg: running some chunk of the test suite with opaque pointers forced on, watching what tests fail, pick one, add a run line to force opaque pointers on, fixing the code until it passes. Rather than finding getElementPointer calls and changing them directly based on source inspection & existing regression testing only.

This revision is now accepted and ready to land.Jul 13 2021, 10:28 AM
This revision was landed with ongoing or failed builds.Aug 19 2021, 9:56 AM
This revision was automatically updated to reflect the committed changes.

I got a second opinion from rnk and he agreed that it's probably not necessary to add tests unless we're adding a new code path.