This is an archive of the discontinued LLVM Phabricator instance.

Differential D146720

[AMDGPU][printf] Run AMDGPUPrintfRuntimeBindingPass in -O0
ClosedPublic

Authored by jmmartinez on Mar 23 2023, 6:54 AM.

Details

Reviewers
jdoerfert
arsenm
Commits
rG215cfa01f2ba: [AMDGPU][printf] Run AMDGPUPrintfRuntimeBindingPass in -O0
Summary

AMDGPUPrintfRuntimeBindingPass is not run in the IR optimization
pipeline with -O0.

This means that with OpenCL the printf definition coming from
device_libs gets linked with the user's code, which blocks
AMDGPUPrintfRuntimeBindingPass from working after the linkage is done.

This patch also adds an opt-pipeline.ll test inspired from
llc-pipeline.ll to document the optimization pipeline.

Diff Detail

Event Timeline

jmmartinez created this revision.Mar 23 2023, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 6:54 AM
Herald added subscribers: kosarev, foad, kerbowa and 8 others. · View Herald Transcript
jmmartinez requested review of this revision.Mar 23 2023, 6:54 AM
Herald added a reviewer: jdoerfert. · View Herald TranscriptMar 23 2023, 6:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, jplehr, • pcwang-thead and 2 others. · View Herald Transcript
jmmartinez added a reviewer: arsenm.Mar 23 2023, 6:54 AM
jmmartinez added inline comments.
llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
18 ↗(On Diff #507727)

I have the feeling that I should remove analysis and analysis invalidations from the list.

Harbormaster completed remote builds in B221304: Diff 507727.Mar 23 2023, 7:53 AM
arsenm accepted this revision.Mar 23 2023, 9:48 AM

LGTM, thanks I noticed this when I was fixing printf recently but forgot to fix this part

llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
18 ↗(On Diff #507727)

Depends, do theses passes actually do anything? I don’t know what CoroConditionalWrappper does

This revision is now accepted and ready to land.Mar 23 2023, 9:48 AM
jmmartinez updated this revision to Diff 508011.Mar 24 2023, 2:34 AM
  • Updating opt-pipeline.ll test to validate on the CI
Harbormaster completed remote builds in B221520: Diff 508011.Mar 24 2023, 3:27 AM
arsenm added inline comments.Mar 24 2023, 4:36 AM
llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
18 ↗(On Diff #507727)

By remove I thought you meant stop the pass from running. In terms of testing, I expect every single pass run to be tested here and every check use -NEXT

jmmartinez added inline comments.Mar 24 2023, 5:12 AM
llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
18 ↗(On Diff #507727)

Oh sorry, I actually meant the removing the "Running analysis" and "Invalidating analysis" entries, such that only the code transformation are checked. And, since I removed the analysis entries, I've added the I've added the --implicit-check-not="Running pass" flag instead of using CHECK-NEXT.

I can revert these changes if you prefer.

foad added a subscriber: aeubanks.Mar 24 2023, 5:51 AM
foad added inline comments.
llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
1 ↗(On Diff #508011)

See previous discussion of why this file was removed and what it should be replaced with: https://github.com/llvm/llvm-project/commit/6f73bd781305266a747055875ce8352e5a36c809

Adding @aeubanks.

aeubanks added a comment.Mar 24 2023, 7:48 AM

could we not put the full pipeline as a test and just check that relevant passes run?

jmmartinez added a comment.Mar 24 2023, 8:19 AM
In D146720#4219568, @aeubanks wrote:

could we not put the full pipeline as a test and just check that relevant passes run?

Sounds good to me. There is a test already checking this (well, not exactly, it checks that the transformation done by the pass is done).

I'm going to remove the opt-pipeline test, it seems that most of the problems are coming from there.

jmmartinez updated this revision to Diff 508109.Mar 24 2023, 8:28 AM
  • Removed the opt-pipeline.ll test.
arsenm accepted this revision.Mar 24 2023, 8:44 AM

LGTM with test nit

llvm/test/CodeGen/AMDGPU/opencl-printf-pipeline.ll
13

This will be a really fragile negative check, just use generated checks for the output

15

Don't think you need the (ptr addrspace(4), ...) here

jmmartinez updated this revision to Diff 508129.Mar 24 2023, 9:15 AM
  • Update test to use auto generated checks.
jmmartinez marked 2 inline comments as done.Mar 24 2023, 9:16 AM
  • Took into account remarks.
aeubanks added a comment.Mar 24 2023, 9:17 AM

this is fine, but I meant that you could still do the -debug-pass-manager run, just only check for AMDGPUPrintfRuntimeBindingPass instead of the whole pipeline

Harbormaster completed remote builds in B221618: Diff 508129.Mar 24 2023, 10:48 AM
Closed by commit rG215cfa01f2ba: [AMDGPU][printf] Run AMDGPUPrintfRuntimeBindingPass in -O0 (authored by jmmartinez). · Explain WhyMar 27 2023, 12:44 AM
This revision was automatically updated to reflect the committed changes.
jmmartinez added a commit: rG215cfa01f2ba: [AMDGPU][printf] Run AMDGPUPrintfRuntimeBindingPass in -O0.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
3 lines
test/
CodeGen/
AMDGPU/
30 lines

Diff 508509

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

Show First 20 Lines • Show All 681 Lines • ▼ Show 20 Lines PB.registerPipelineStartEPCallback(
FPM.addPass(AMDGPUUseNativeCallsPass()); FPM.addPass(AMDGPUUseNativeCallsPass());
if (EnableLibCallSimplify && Level != OptimizationLevel::O0) if (EnableLibCallSimplify && Level != OptimizationLevel::O0)
FPM.addPass(AMDGPUSimplifyLibCallsPass(*this)); FPM.addPass(AMDGPUSimplifyLibCallsPass(*this));
PM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM))); PM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
}); });
PB.registerPipelineEarlySimplificationEPCallback( PB.registerPipelineEarlySimplificationEPCallback(
[this](ModulePassManager &PM, OptimizationLevel Level) { [this](ModulePassManager &PM, OptimizationLevel Level) {
PM.addPass(AMDGPUPrintfRuntimeBindingPass());
if (Level == OptimizationLevel::O0) if (Level == OptimizationLevel::O0)
return; return;
PM.addPass(AMDGPUUnifyMetadataPass()); PM.addPass(AMDGPUUnifyMetadataPass());
PM.addPass(AMDGPUPrintfRuntimeBindingPass());
if (InternalizeSymbols) { if (InternalizeSymbols) {
PM.addPass(InternalizePass(mustPreserveGV)); PM.addPass(InternalizePass(mustPreserveGV));
} }
PM.addPass(AMDGPUPropagateAttributesLatePass(*this)); PM.addPass(AMDGPUPropagateAttributesLatePass(*this));
if (InternalizeSymbols) { if (InternalizeSymbols) {
PM.addPass(GlobalDCEPass()); PM.addPass(GlobalDCEPass());
} }
▲ Show 20 Lines • Show All 919 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/opencl-printf-pipeline.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
; RUN: opt -mtriple=amdgcn--amdhsa -passes=amdgpu-printf-runtime-binding -S < %s | FileCheck %s
; RUN: opt -mtriple=amdgcn--amdhsa -O0 -S < %s | FileCheck %s
; Check that the call to printf is removed when AMDGPUPrintfRuntimeBindingPass is executed
; And that this pass is executed in the -O0 and -OX pipelines
@.str = private unnamed_addr addrspace(4) constant [7 x i8] c"hello\0A\00", align 1
define void @foo() {
; CHECK-LABEL: define void @foo() {
; CHECK-NEXT: [[PRINTF_ALLOC_FN:%.*]] = call ptr addrspace(1) @__printf_alloc(i32 4)
; CHECK-NEXT: br label [[DOTSPLIT:%.*]]
arsenmUnsubmitted
Done
ReplyInline Actions

This will be a really fragile negative check, just use generated checks for the output

arsenm: This will be a really fragile negative check, just use generated checks for the output
; CHECK: .split:
; CHECK-NEXT: [[TMP1:%.*]] = icmp ne ptr addrspace(1) [[PRINTF_ALLOC_FN]], null
arsenmUnsubmitted
Done
ReplyInline Actions

Don't think you need the (ptr addrspace(4), ...) here

arsenm: Don't think you need the (ptr addrspace(4), ...) here
; CHECK-NEXT: br i1 [[TMP1]], label [[TMP2:%.*]], label [[TMP3:%.*]]
; CHECK: 2:
; CHECK-NEXT: [[PRINTBUFFID:%.*]] = getelementptr i8, ptr addrspace(1) [[PRINTF_ALLOC_FN]], i32 0
; CHECK-NEXT: [[PRINTBUFFIDCAST:%.*]] = bitcast ptr addrspace(1) [[PRINTBUFFID]] to ptr addrspace(1)
; CHECK-NEXT: store i32 1, ptr addrspace(1) [[PRINTBUFFIDCAST]], align 4
; CHECK-NEXT: [[PRINTBUFFGEP:%.*]] = getelementptr i8, ptr addrspace(1) [[PRINTF_ALLOC_FN]], i32 4
; CHECK-NEXT: br label [[TMP3]]
; CHECK: 3:
; CHECK-NEXT: ret void
;
%call = call i32 @printf(ptr addrspace(4) @.str)
ret void
}
declare hidden i32 @printf(ptr addrspace(4), ...)