Page MenuHomePhabricator

[Polly] [PPCGCodeGeneration] Allow intrinsics within kernels.
ClosedPublic

Authored by bollu on Jun 13 2017, 6:25 AM.

Details

Summary
  • In D33414, if any function call was found within a kernel, we would bail out.
  • This is an over-approximation. This patch changes this by allowing intrinsics within kernels.
  • This introduces an additional step when creating a separate llvm::Module for a kernel. We now have to copy the intrinsics from the original llvm::Module to the kernel-specific module.
  • Note that this maybe too lax. Perhaps we are allowing intrinsics which should not be allowed through. This can be debated in the review.

Diff Detail

Repository
rL LLVM

Event Timeline

bollu created this revision.Jun 13 2017, 6:25 AM
bollu added a comment.Jun 13 2017, 8:38 AM

the TODO blocks are commentary where I would like some discussion before merging this revision.

test/GPGPU/intrinsic-copied-into-kernel.ll
11 ↗(On Diff #102326)

Just realised this: I should also check for the declare of sqrt. Will add.

bollu added a comment.Jun 15 2017, 5:45 AM

@singam-sanjay: We had discussed trying this out on the Julia code with debug intrinsics yesterday. Were you able to try it out? Thanks!

singam-sanjay edited edge metadata.Jun 15 2017, 3:05 PM

@bollu The patch did prevent passing the address of the debug intrinsic to the kernel, but couldn't guard against buggy metadata,

DICompileUnit not listed in llvm.dbg.cu
!11 = distinct !DICompileUnit(language: DW_LANG_C89, file: !3, producer: "julia", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !12)

I'll have to look closer at how *GPUModule is being produced to understand this error.

bollu added a comment.Jun 17 2017, 9:54 AM

I lack context, I think. Where is wrong metadata coming from?
Also, FWIW, I would not add code that guards against illegal metadata.

@bollu Please refer this comment on my patch. I observed a pattern with functions that produced the buggy metadata, simple copy kernels (A[i][j][k] = B[i][j][k]) didn't produce buggy metadata but kernels that had computation (A[i][j][k] +=/-=/*= B[i][j][k]) resulted in Modules that didn't list the DICompileUnit in llvm.dbg.cu.

I'm not sure how a module that's built independent from source IR carries metadata. Do you think there'd be a function to explicitly list the DICompileUnit in llvm.dbg.cu ?

grosser edited edge metadata.Jun 17 2017, 11:32 PM

I believe the Julia problem consists of two (partially independent) bugs. If this patch fixes a part of your problem (the intrinsics), then we should likely commit it and thing about the metadata afterwards.

Yes, we can handle the metadata problem in a different patch.

bollu updated this revision to Diff 103191.Jun 20 2017, 5:51 AM
  • [NFC] typo fix.
  • Try to retreive pre-existing function. If this does not exist, call Function::Create.
  • [NFC] add a debug message if we are bailing out from PPCGCodeGeneration due to an unrecognised function in a kernel.
bollu updated this revision to Diff 103863.Jun 25 2017, 3:09 AM
  • [NFC] documented replaceKernelFunctions.
bollu updated this revision to Diff 103923.Jun 26 2017, 4:48 AM
  • Changed approach as discussed offline with Tobias. It was decided to use the BlockGenerator functionality to setup correct kernel code generation, rather than fixing generated code.
  • General patch cleanup.

Some more comments.

lib/CodeGen/PPCGCodeGeneration.cpp
1120 ↗(On Diff #102326)

In llvm we start function names with lowercase letters.

Also, please document what the parameter F does.

Also, the comment is not useful. Please describe in one sentence what the function does, and then in a small paragraph what the criteria which a function that is listed here must fullfill.

1122 ↗(On Diff #102326)

Please make this an explicit list of intrinsics. Maybe start with sqrt only to extablish the infrastructure.

2704 ↗(On Diff #102326)

support
start with lowercase letters (it seems I missed this in the last review)

Also, in general we comment functions as follow:

/// Brief one-line comment
///
/// Longer paragraph explaining what is happening here.
2816 ↗(On Diff #102326)

Do not add braces.

grosser added inline comments.Jun 26 2017, 5:02 AM
lib/CodeGen/PPCGCodeGeneration.cpp
401 ↗(On Diff #103923)

Create new function DECLARATIONS ...

408 ↗(On Diff #103923)

Should be IslNodeBuilder::ValueMap

1136 ↗(On Diff #103923)

Start with a comment describing the function.

1138 ↗(On Diff #103923)

Drop TODO.

1122 ↗(On Diff #102326)

Please add a comment that we match any llvm.sqrt intrinsics by doing a string match.

bollu updated this revision to Diff 103928.Jun 26 2017, 5:15 AM
  • [NFC] style fixes.
bollu updated this revision to Diff 103931.Jun 26 2017, 5:21 AM
  • Fix function capitalization, spelling and comments
bollu added inline comments.Jun 26 2017, 5:23 AM
lib/CodeGen/PPCGCodeGeneration.cpp
1136 ↗(On Diff #103923)

This is a member function, so it is already documented at GPUNodeBuilder declaration.

2816 ↗(On Diff #102326)

I do need braces because the check contains (potentially) two statements in the body?

grosser added inline comments.Jun 26 2017, 5:23 AM
lib/CodeGen/PPCGCodeGeneration.cpp
374 ↗(On Diff #103928)

Please document the new parameter as well.

bollu updated this revision to Diff 103932.Jun 26 2017, 5:26 AM
  • [NFC] document getFunctionsFromRawSubtreeValues.
grosser accepted this revision.Jun 26 2017, 5:29 AM

LGTM. Please document the new parameter.

This revision is now accepted and ready to land.Jun 26 2017, 5:29 AM
This revision was automatically updated to reflect the committed changes.