Page MenuHomePhabricator

[CSSPGO] MIR target-independent pseudo instruction for pseudo-probe intrinsic
ClosedPublic

Authored by hoy on Aug 24 2020, 4:56 PM.

Details

Summary

This change introduces a MIR target-independent pseudo instruction corresponding to the IR intrinsic llvm.pseudoprobe for pseudo-probe block instrumentation. Please refer to https://reviews.llvm.org/D86193 for the whole story.

An llvm.pseudoprobe intrinsic call will be lowered into a target-independent operation named PSEUDO_PROBE. Given the following instrumented IR,

define internal void @foo2(i32 %x, void (i32)* %f) !dbg !4 {
bb0:
   %cmp = icmp eq i32 %x, 0
   call void @llvm.pseudoprobe(i64 837061429793323041, i64 1)
   br i1 %cmp, label %bb1, label %bb2
bb1:                                             
   call void @llvm.pseudoprobe(i64 837061429793323041, i64 2)
   br label %bb3
bb2:                                              
   call void @llvm.pseudoprobe(i64 837061429793323041, i64 3)
   br label %bb3
bb3:                                              
   call void @llvm.pseudoprobe(i64 837061429793323041, i64 4)
   ret void
}

the corresponding MIR is shown below. Note that block bb3 is duplicated into bb1 and bb2 where its probe is duplicated too. This allows for an accurate execution count to be collected for bb3, which is basically the sum of the counts of bb1 and bb2.

bb.0.bb0:
   frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
   TEST32rr killed renamable $edi, renamable $edi, implicit-def $eflags
   PSEUDO_PROBE 837061429793323041, 1, 0
   $edi = MOV32ri 1, debug-location !13; test.c:0
   JCC_1 %bb.1, 4, implicit $eflags

bb.2.bb2:
   PSEUDO_PROBE 837061429793323041, 3, 0
   PSEUDO_PROBE 837061429793323041, 4, 0
   $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp
   RETQ

bb.1.bb1:
   PSEUDO_PROBE 837061429793323041, 2, 0
   PSEUDO_PROBE 837061429793323041, 4, 0
   $rax = frame-destroy POP64r implicit-def $rsp, implicit $rsp
   RETQ

The target op PSEUDO_PROBE will be converted into a piece of binary data by the object emitter with no machine instructions generated. This is done in a different patch.

Diff Detail

Event Timeline

hoy created this revision.Aug 24 2020, 4:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 4:56 PM
hoy requested review of this revision.Aug 24 2020, 4:56 PM
hoy edited the summary of this revision. (Show Details)Aug 24 2020, 5:03 PM
hoy edited the summary of this revision. (Show Details)
hoy updated this revision to Diff 287524.Aug 24 2020, 5:14 PM

Updating D86495: [CSSPGO] MIR target-independent pseudo instruction for pseudo-probe intrinsic

hoy added a comment.Aug 24 2020, 5:26 PM

@wmi Do you know how to chain this patch with my previous patch https://reviews.llvm.org/D86490 so that the two patches can be tested together? Thanks.

I don't know since I don't use arc.

wmi added a comment.Sep 9 2020, 9:02 PM

It is better to have a MIR test.

hoy added a comment.Sep 9 2020, 9:46 PM
In D86495#2264868, @wmi wrote:

It is better to have a MIR test.

Yeah, the test is actually included in https://reviews.llvm.org/D86499. It tests both the IR and MIR change.

wmi added a comment.Sep 9 2020, 9:51 PM
In D86495#2264877, @hoy wrote:
In D86495#2264868, @wmi wrote:

It is better to have a MIR test.

Yeah, the test is actually included in https://reviews.llvm.org/D86499. It tests both the IR and MIR change.

I mean to have a .mir test which read MIR as input. That is to ensure llc read MIR text containing the new pseudo probe without problem.

hoy added a comment.Sep 10 2020, 11:03 AM
In D86495#2264881, @wmi wrote:
In D86495#2264877, @hoy wrote:
In D86495#2264868, @wmi wrote:

It is better to have a MIR test.

Yeah, the test is actually included in https://reviews.llvm.org/D86499. It tests both the IR and MIR change.

I mean to have a .mir test which read MIR as input. That is to ensure llc read MIR text containing the new pseudo probe without problem.

I see. Added a .mir test.

hoy updated this revision to Diff 291031.Sep 10 2020, 11:04 AM

Updating D86495: [CSSPGO] MIR target-independent pseudo instruction for pseudo-probe intrinsic

hoy updated this revision to Diff 306476.Nov 19 2020, 10:42 AM

Updating D86495: [CSSPGO] MIR target-independent pseudo instruction for pseudo-probe intrinsic

Adding an attribute field for use later.

wmi accepted this revision.Nov 19 2020, 10:47 PM
This revision is now accepted and ready to land.Nov 19 2020, 10:47 PM
This revision was landed with ongoing or failed builds.Nov 20 2020, 10:53 AM
This revision was automatically updated to reflect the committed changes.
mceier added a subscriber: mceier.Nov 24 2020, 12:10 AM
mceier added inline comments.
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

The size of this class (66 if I computed it correctly) exceeds the size of LargestSDNode (64 bytes) in llvm/include/llvm/CodeGen/SelectionDAGNodes.h causing static_assert to fail in Recycler.h when compiling SelectionDAG.cpp (at least) on 32-bit system:

In file included from llvm/include/llvm/CodeGen/MachineFunction.h:35,
                 from llvm/include/llvm/CodeGen/SelectionDAG.h:31,
                 from llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:13:
llvm/include/llvm/Support/Recycler.h: In instantiation of ‘SubClass* llvm::Recycler<T, Size, Align>::Allocate(AllocatorType&) [with SubClass = llvm::PseudoProbeSDNode; AllocatorType = llvm::BumpPtrAllocatorImpl<>; T = llvm::SDNode; unsigned int Size = 64; unsigned int Align = 4]’:
llvm/include/llvm/Support/RecyclingAllocator.h:43:65:   required from ‘SubClass* llvm::RecyclingAllocator<AllocatorType, T, Size, Align>::Allocate() [with SubClass = llvm::PseudoProbeSDNode; AllocatorType = llvm::BumpPtrAllocatorImpl<>; T = llvm::SDNode; unsigned int Size = 64; unsigned int Align = 4]’
llvm/include/llvm/CodeGen/SelectionDAG.h:380:57:   required from ‘SDNodeT* llvm::SelectionDAG::newSDNode(ArgTypes&& ...) [with SDNodeT = llvm::PseudoProbeSDNode; ArgTypes = {const unsigned int&, unsigned int, const llvm::DebugLoc&, const llvm::SDVTList&, long long unsigned int&, long long unsigned int&, unsigned int&}]’
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6906:72:   required from here
llvm/include/llvm/Support/Recycler.h:86:36: error: static assertion failed: Recycler allocation size is less than object size!
   86 |     static_assert(sizeof(SubClass) <= Size,
      |                   ~~~~~~~~~~~~~~~~~^~~~~~~

Relevant bug: https://bugs.llvm.org/show_bug.cgi?id=48259

I'm guessing this code in llvm/include/llvm/CodeGen/SelectionDAGNodes.h:

using LargestSDNode = AlignedCharArrayUnion<AtomicSDNode, TargetIndexSDNode,
                                            BlockAddressSDNode,
                                            GlobalAddressSDNode>;

should be changed to:

using LargestSDNode = AlignedCharArrayUnion<AtomicSDNode, TargetIndexSDNode,
                                            BlockAddressSDNode,
                                            GlobalAddressSDNode,
                                            PseudoProbeSDNode>;
hoy added inline comments.Nov 24 2020, 9:33 AM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

Thanks for reporting the bug! I'm sorry for the breakage. Will send out a fix.

hoy added inline comments.Nov 24 2020, 12:37 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

@mceier Could you do a favor by giving your suggested fix a try? I just realized that my system doesn't have a 32-bit toolset. Thanks!

mceier added inline comments.Nov 24 2020, 1:04 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

Yes. SelectionDAG.cpp compiled with that change, I'm still waiting for whole build to finish.

hoy added inline comments.Nov 24 2020, 1:06 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

Thanks for the update! Please let me know once the whole build is fine and I'll push the fix in.

mceier added inline comments.Nov 24 2020, 3:28 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

Just finished building llvm - both 32-bit and 64-bit versions build with this change.

hoy added inline comments.Nov 24 2020, 3:37 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1752–1777

Thanks for the confirmation! I just pushed the change in.