This is an archive of the discontinued LLVM Phabricator instance.

[openmp][libomptarget][nfc] Infer types instead of duplicate and cast void
Needs ReviewPublic

Authored by JonChesterfield on Dec 17 2021, 12:22 PM.

Details

Summary

Replaces handwritten typedefs and casts via void* with types inferred
from declarations in omptargetplugins.h. Leaves unchanged a function with the
wrong type and register/unregister which follow a different pattern.

Rename a couple of pointer variables to match the api (missing _target_
Minor change made to dlwrap.h to use the type inference from it.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Dec 17 2021, 12:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2021, 12:22 PM

There's more that can be done here in terms of generating the wrappers from the header but this gets rid of the handwritten typedefs without disturbing much else.

openmp/libomptarget/include/dlwrap.h
74 ↗(On Diff #395196)

Without this or similar, including dlwrap.h from a file that doesn't use all the pieces errors about declared but missing static functions. I'm considering moving the non-dlwrap parts into a separate header but leaving that for a later patch.

openmp/libomptarget/include/omptargetplugin.h
17

Probably harmless, but would like to use <> or "" consistently when referring to this. rtl.h uses "". Don't really want this header to find an omptarget from the system if it's locally installed so "" seems safer

openmp/libomptarget/src/device.cpp
500–502

This (and three similar) pointers don't follow the pattern of plugin API with __tgt_rtl prefix removed, renamed them.

openmp/libomptarget/src/rtl.h
28

Whole patch is a reaction to discovering this sequence of typedefs in rtl.h. Way too easy to get this wrong and no need to type them out by hand.

JonChesterfield retitled this revision from [openmp][libomptarget][nfc Infer types instead of duplicate and cast void to [openmp][libomptarget][nfc] Infer types instead of duplicate and cast void.Dec 17 2021, 2:21 PM
JonChesterfield added inline comments.
openmp/libomptarget/include/dlwrap.h
74 ↗(On Diff #395196)

I think the change to dlwrap is good in it's own right, we shouldn't have a header that is an error to include unless some macro in it is invoked and the INITIALIZE() call may be useful anyway. Got some other patches backing up on this so will land that separate to the libomptarget implementation change

tianshilei1992 added inline comments.Jan 7 2022, 12:36 PM
openmp/libomptarget/src/device.cpp
500–502

worth doing it in another patch

openmp/libomptarget/src/rtl.h
83

load_binary is left.

86

I think these changes also worth a separate patch.