This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][NFC] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU
ClosedPublic

Authored by saiislam on Jul 13 2020, 2:49 PM.

Details

Summary

Refactors CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU to make it a
generalization for OpenMP GPU Codegen. Target specific specialized
methods for NVPTX are defined in class CGOpenMPRuntimeNVPTX. This
paves the way for a clean and maintainable extension to more GPU
targets for OpenMP Codegen.

For original author (git blame) list of CGOpenMPRuntimeGPU code,
look in history of CGOpenMPRuntimeNVPTX.cpp and .h, after this commit.

Diff Detail

Event Timeline

saiislam created this revision.Jul 13 2020, 2:49 PM

This is indeed the direction I had in mind. Broad strokes look right. I probably wouldn't notice an accidental change amidst the whitespace reshuffle. Very happy to read through line by line if you can split the whitespace change out.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.h
377

It's worth avoiding whitespace-only changes in a large diff, even when it brings the code in line with clang-format's rules. Signal to noise is challenging enough without it.

Please would you leave the whitespace-only changes out? I usually open the diff in a friendly GUI and eyeball each segment to see if it can be dropped.

Feel free to fix the whitespace before or after.

saiislam updated this revision to Diff 277705.Jul 14 2020, 1:41 AM

Removed all formatting and spacing changes introduced due to clang-format.

ABataev added inline comments.Jul 14 2020, 4:03 AM
clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp
39 ↗(On Diff #277705)

This is new functionality, better to move it in a separate patch, and this one mark as NFC.

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp
39 ↗(On Diff #277705)

Works for me. This patch shows how the per-target parts are intended to be done. First patch being totally NFC seems good.

saiislam updated this revision to Diff 277824.Jul 14 2020, 7:18 AM

Removed all references to AMDGCN to make it NFC. Will introduce AMDGCN specific things separately in a followup patch.

saiislam retitled this revision from [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU to [OpenMP][NFC] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU.Jul 14 2020, 7:19 AM
saiislam edited the summary of this revision. (Show Details)
ABataev accepted this revision.Jul 14 2020, 8:01 AM

I meant to remove just the new virtual function and its new implementation for AMDGCN, you could keep the new class for AMD GPUs. But I'm fine with this too.

This revision is now accepted and ready to land.Jul 14 2020, 8:01 AM

Thank you.
I will hold committing till llvm-11 branching so it doesn't break something in any downstream dependent project at the last moment.

This revision was automatically updated to reflect the committed changes.
clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.h