Page MenuHomePhabricator

Prefix the name of the calling host function in the name of callee GPU kernel
ClosedPublic

Authored by singam-sanjay on Jun 7 2017, 5:21 AM.

Details

Summary

Provide more context to the name of a GPU kernel by prefixing its name with the host function that calls it. E.g. The first kernel called by gemm would be FUNC_gemm_KERNEL_0.

Kernels currently follow the "kernel_#" (# = 0,1,2,3,...) nomenclature. This patch makes it easier to map host caller and device callee, especially when there are many kernels produced by Polly-ACC.

Diff Detail

Repository
rL LLVM

Event Timeline

singam-sanjay created this revision.Jun 7 2017, 5:21 AM
singam-sanjay edited the summary of this revision. (Show Details)Jun 7 2017, 5:53 AM
singam-sanjay edited the summary of this revision. (Show Details)Jun 7 2017, 11:24 AM
grosser edited edge metadata.Jun 7 2017, 2:33 PM

Nice. The summary reads well. Please add a test case and this should be ready to commit!

Updated exiting test cases to reflect effect of the patch.

I've corrected the test cases which are affected by the patch. Should I include a separate patch to highlight the effects of just this patch ?

grosser accepted this revision.Jun 9 2017, 4:16 AM

LGTM.

Can you please ask Chris to give you commit access?

This revision is now accepted and ready to land.Jun 9 2017, 4:16 AM
singam-sanjay retitled this revision from Prefix the name of the calling host function in the name of callee GPU kernel to Prefix the name of the calling host function and Scop in the name of callee GPU kernel.
singam-sanjay edited the summary of this revision. (Show Details)

Prefix SCoP name as well to uniquely identify a kernel, since a function can have more than one SCoP.

singam-sanjay requested review of this revision.Jun 21 2017, 10:13 AM
singam-sanjay edited edge metadata.

@grosser ping.

singam-sanjay edited the summary of this revision. (Show Details)
  1. Clearly mention the source FUNCtion and SCOP and KERNEL number.
  2. Changed name of SCOP prefixed to kernel to follow PTX identifier syntax acc. to http://docs.nvidia.com/cuda/parallel-thread-execution/index.html#identifiers (Raised by @philip.pfaffe )

@kbarton This patch doesn't have anything to do with the PowerPC backend. Could you please update your rules to search for "PowerPC" (might be in the comments) or drop files with "PPCGCodeGeneration" ?

bollu edited edge metadata.EditedJun 25 2017, 3:05 AM

@singam-sanjay I've added style comments. Feel free to ignore the const stuff, but please do remove the code duplication. Other than that, LGTM.

I feel very strongly about immutability, but I understand that using const irregularly is worse than not having const at all. However, I'd like to have const in as many low-friction places as possible, simply because it makes reading code nicer :)

lib/CodeGen/PPCGCodeGeneration.cpp
209 ↗(On Diff #103753)

Have you considered const std::string? Since we know that this is immutable, we can initialise it once in the constructor of GPUNodeBuilder.

242 ↗(On Diff #103753)

Could this be a static function? static string createScopNameForIR(const Scop &S). I feel that this carries intent better, because it does not use the state of GPUNodeBuilder at all.

Then, the constructor of GPUNodeBuilder can be

GPUNodeBuilder::GPUNodeBuilder(Scop &S, ...) : ScopNameForIR(createScopNameForIR(S)), ... { }
1492 ↗(On Diff #103753)

Name creation seems to be replicated here and on line 1450, line 1757. Can this be factored out into a common function?

static std::string CreateKernelIdentifier(ppcg_kernel *Kernel, const Scop &S) { 
   return  "FUNC_" + S.getFunction().getName().str() + 
           "_SCOP_" + getScopNameForIR() + "_KERNEL_" + 
            std::to_string(Kernel->id);
}
singam-sanjay added inline comments.Jun 29 2017, 1:04 AM
lib/CodeGen/PPCGCodeGeneration.cpp
209 ↗(On Diff #103753)

I'll change it. Do you think ScopNameForIR should be made a member of Scop ? ( I could add this to D34565 )

242 ↗(On Diff #103753)

Yes, I agree. Instead, can I make this function a part of class Scop ? Since this is only associated with a Scop object.

1492 ↗(On Diff #103753)

Sure. Do you think this should be a part of GPUNodeBuilder ?

Hi Sanjay,

adding the scop's name is a good idea. Specifically, the scop's name may differ between release and debug builds, as names are only carried over in debug builds. Having the compiler's output depend on the build settings is something we avoid in LLVM. If we want to distigush scops in the same function, but different scops we probably want to number the scops / regions, rather than using their names. I suggest we do this in another patch, to not further hold this patch back.

Sorry for the delay.

I meant, adding the scop's name is _not_ a good idea.

singam-sanjay added a comment.EditedJul 1 2017, 9:27 PM

Hi Sanjay,

adding the scop's name is a not good idea. Specifically, the scop's name may differ between release and debug builds, as names are only carried over in debug builds.

The name of the Scop is built from the boundary basic blocks of the region in Scop::getNameStr. How does that depend on the build type ? Are extra basic blocks inserted in debug builds ?

AFAIK, LLVM Debug build lets us stepthrough and debug the LLVM source.

Having the compiler's output depend on the build settings is something we avoid in LLVM.
If we want to distigush scops in the same function, but different scops we probably want to number the scops / regions, rather than using their names. I suggest we do this in another patch, to not further hold this patch back.

Sorry for the delay.

  1. Moved building the name into a routine.
  2. ScopNameofIR is const now.
  3. Rebased to trunk @ 306540.

Hi Sanjay,

adding the scop's name is a not good idea. Specifically, the scop's name may differ between release and debug builds, as names are only carried over in debug builds.

The name of the Scop is built from the boundary basic blocks of the region in Scop::getNameStr. How does that depend on the build type ? Are extra basic blocks inserted in debug builds ?

AFAIK, LLVM Debug build lets us stepthrough and debug the LLVM source.

clang (and LLVM) will name basic blocks, but it will not carry name information in release mode. I suggest to remove the SCOP names for now, commit the majority of the patch and open a new discussion for scop names.

Just run '-S -emit-llvm' on a simple program, once using a clang version compiled with BUILD_TYPE=Debug and once with BUILD_TYPE=Release.

Best,
Tobias

Having the compiler's output depend on the build settings is something we avoid in LLVM.
If we want to distigush scops in the same function, but different scops we probably want to number the scops / regions, rather than using their names. I suggest we do this in another patch, to not further hold this patch back.

Sorry for the delay.

grosser requested changes to this revision.Jul 3 2017, 9:05 PM
This revision now requires changes to proceed.Jul 3 2017, 9:05 PM
singam-sanjay edited edge metadata.

Drop Scop name from ptx_kernel's name.

singam-sanjay retitled this revision from Prefix the name of the calling host function and Scop in the name of callee GPU kernel to Prefix the name of the calling host function in the name of callee GPU kernel.Jul 4 2017, 11:16 AM
singam-sanjay edited the summary of this revision. (Show Details)
singam-sanjay set the repository for this revision to rL LLVM.
grosser accepted this revision.Jul 4 2017, 12:02 PM

LGTM, thanks

This revision was automatically updated to reflect the committed changes.