[SampleFDO] Add two passes of MIRAddFSDiscriminatorsPass

[xur]

Summary

This patch adds Pass1 of MIRADDFSDiscriminatorsPass before register allocation, and
Pass2 of MIRAddFSDiscriminatorsPass before Block-Placement.
This is still under --enable-fs-discrmininator option (default false).

This would reduce the turn-around time for FSAFDO transition.

[wenlei]

This would reduce the turn-around time for FSAFDO transition.

Not sure if I follow, could you explain a bit more?

llvm/lib/CodeGen/TargetPassConfig.cpp

1468

Wondering once we finalize the spots for adding FS discriminators, do we want to replace Pass1, Pass2.. to say BBPlacementDisc, etc. eventually

llvm/test/CodeGen/X86/fsafdo_test1.ll

6

This is changing from (input discriminator + last discriminator) to (input discriminator + block placement discriminator + last discriminator), right?

[xur]

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

[wenlei]

In D104579#2828504, @xur wrote:

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

[xur]

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

That's a good suggestion. I haven't tried to experiment with one pass before RA. I know that the one before block placement will the performance. I also tried add one after block-placement but did not gain much.
Let me do some experiments and report back.

Another aspect of this change is that we want to do this more conservatively. We have many SampleFDO targets and we don't want to commit too much changes in the initial flip ( to minimize compile time, memory usage impact). We prefer to do this step by step.

[wenlei]

In D104579#2829676, @xur wrote:

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

That's a good suggestion. I haven't tried to experiment with one pass before RA. I know that the one before block placement will the performance. I also tried add one after block-placement but did not gain much.
Let me do some experiments and report back.

Another aspect of this change is that we want to do this more conservatively. We have many SampleFDO targets and we don't want to commit too much changes in the initial flip ( to minimize compile time, memory usage impact). We prefer to do this step by step.

Incremental approach sounds reasonable. Was just curious whether you have tried other passes and see no improvements.

Looks like FS-AFDO changes are mostly in tree now though the profile generation part for FS profile is still outside of llvm (in your create_llvm_prof tool), right? Otherwise, we can give it a try too.

[wenlei]

llvm/lib/CodeGen/TargetPassConfig.cpp

1468

An extra profile loader pass is still to be added here coupled with the FS disc pass?

[hoy]

llvm/lib/CodeGen/TargetPassConfig.cpp

1468

addBlockPlacement is very close to addPreEmitPass in pass order. I'm wondering if we should use FSDiscriminatorPass::Pass3.

[xur]

In D104579#2831094, @wenlei wrote:

In D104579#2829676, @xur wrote:

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

That's a good suggestion. I haven't tried to experiment with one pass before RA. I know that the one before block placement will the performance. I also tried add one after block-placement but did not gain much.
Let me do some experiments and report back.

Another aspect of this change is that we want to do this more conservatively. We have many SampleFDO targets and we don't want to commit too much changes in the initial flip ( to minimize compile time, memory usage impact). We prefer to do this step by step.

Incremental approach sounds reasonable. Was just curious whether you have tried other passes and see no improvements.

Looks like FS-AFDO changes are mostly in tree now though the profile generation part for FS profile is still outside of llvm (in your create_llvm_prof tool), right? Otherwise, we can give it a try too.

Most code is in except the FS_ProfileLoader I can send FS ProfileLoad for review (can be turned on by an option).

I think I checked in all of code for create_llvm_prof tool internally. We just need to send it to upstream some time. I'll discuss with Wei.
But I heard you guys do not use create-llvm-prof tool. Is that right?

[wenlei]

In D104579#2831709, @xur wrote:

In D104579#2831094, @wenlei wrote:

In D104579#2829676, @xur wrote:

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

That's a good suggestion. I haven't tried to experiment with one pass before RA. I know that the one before block placement will the performance. I also tried add one after block-placement but did not gain much.
Let me do some experiments and report back.

Another aspect of this change is that we want to do this more conservatively. We have many SampleFDO targets and we don't want to commit too much changes in the initial flip ( to minimize compile time, memory usage impact). We prefer to do this step by step.

Incremental approach sounds reasonable. Was just curious whether you have tried other passes and see no improvements.

Looks like FS-AFDO changes are mostly in tree now though the profile generation part for FS profile is still outside of llvm (in your create_llvm_prof tool), right? Otherwise, we can give it a try too.

Most code is in except the FS_ProfileLoader I can send FS ProfileLoad for review (can be turned on by an option).

I think I checked in all of code for create_llvm_prof tool internally. We just need to send it to upstream some time. I'll discuss with Wei.
But I heard you guys do not use create-llvm-prof tool. Is that right?

Yeah, we don't use the create_llvm_prof tool from google's github. We have our own internal implementation which is similar, but we want to move everything into llvm-profgen inside llvm-project and switch to use llvm-profgen in production too (@wlei will help on that in H2). Hopefully that will make it easier for all of us to share things in the future.

If you can upstream FS part of create_llvm_prof, it'd be very helpful too, and we can also help integrated that into llvm-profgen if needed down the road.

[xur]

Add another pass berfore RA (as suggested by WenLei).
I tested the performance for FSAFDO Loader using one google benchmark.
This extra pass turns to be positive in performance -- I'm seeing additional 0.6% - 0.8% improvement.

BTW, sorry for the delay. I was off work last month.

[wmi]

This extra pass turns to be positive in performance -- I'm seeing additional 0.6% - 0.8% improvement.

That is a good improvement. I guess the improvement is because regalloc is profile sensitive and it is benefited from preciser fs profile information after you insert another pass of profile loading before regalloc?

[wenlei]

In D104579#2937770, @wmi wrote:

This extra pass turns to be positive in performance -- I'm seeing additional 0.6% - 0.8% improvement.

That is a good improvement. I guess the improvement is because regalloc is profile sensitive and it is benefited from preciser fs profile information after you insert another pass of profile loading before regalloc?

Good to know, thanks for experimenting Rong! We've also seen RA being sensitive to profile in our profile inference work - sometimes small change in profile due to inference change can lead to significant cascading effect in RA.

[wmi]

llvm/lib/CodeGen/TargetPassConfig.cpp

1116–1118

How about adding a comment that the pass is intentionally placed before regalloc, in case some other passes are inserted in between in the future.

[wenlei]

llvm/lib/CodeGen/TargetPassConfig.cpp

1116–1118

name FSDiscriminatorPass::Pass1 as something like FSDiscriminatorPass::RegAlloc might help too.

[hoy]

Nice to see the win from RegAlloc!

llvm/lib/CodeGen/TargetPassConfig.cpp

1116–1118

Either way sounds good to me. Was wondering if Pass1, Pass2... should be named more specifically by the pass they are preceding. On a second thought, if they are moved around in the future, their name may need be changed too.

[xur]

llvm/lib/CodeGen/TargetPassConfig.cpp

1116–1118

Yes. I have the same concern if we change the names. Let's keep the Pass1 etc for now. Adding a comment definitely helps. Will do.

[wenlei]

lgtm, thanks.

llvm/lib/CodeGen/TargetPassConfig.cpp

void TargetPassConfig::addMachinePasses() {
   // Run pre-ra passes.
   addPreRegAlloc();
 
   // Debugifying the register allocator passes seems to provoke some
   // non-determinism that affects CodeGen and there doesn't seem to be a point
   // where it becomes safe again so stop debugifying here.
   DebugifyIsSafe = false;
 
   // Run register allocation and passes that are tightly coupled with it,
   // including phi elimination and scheduling.
   if (getOptimizeRegAlloc())
     addOptimizedRegAlloc();
   else
     addFastRegAlloc();
 
   // Run post-ra passes.
   addPostRegAlloc();
 
   addPass(&FixupStatepointCallerSavedID);

 /// Add standard GC passes.
 bool TargetPassConfig::addGCPasses() {
   addPass(&GCMachineCodeAnalysisID, false);
   return true;
 }
 
 /// Add standard basic block placement passes.
 void TargetPassConfig::addBlockPlacement() {
+  if (EnableFSDiscriminator)
+    addPass(createMIRAddFSDiscriminatorsPass(
+        sampleprof::FSDiscriminatorPass::Pass1));
   if (addPass(&MachineBlockPlacementID)) {
     // Run a separate pass to collect block placement statistics.
     if (EnableBlockPlacementStats)
       addPass(&MachineBlockPlacementStatsID);
   }
 }
 
 //===---------------------------------------------------------------------===//

llvm/test/CodeGen/X86/fsafdo_test1.ll

 ; RUN: llc -enable-fs-discriminator < %s | FileCheck %s
 ;
 ; Check that fs-afdo discriminators are generated.
 ; CHECK: .loc    1 7 3 is_stmt 0 discriminator 2 # foo.c:7:3
 ; Check: .loc    1 9 5 is_stmt 1 discriminator 2 # foo.c:9:5
-; CHECK: .loc    1 9 5 is_stmt 0 discriminator 268435458 # foo.c:9:5
-; CHECK: .loc    1 7 3 is_stmt 1 discriminator 3892314114 # foo.c:7:3
+; CHECK: .loc    1 9 5 is_stmt 0 discriminator 11266 # foo.c:9:5
+; CHECK: .loc    1 7 3 is_stmt 1 discriminator 11266 # foo.c:7:3
 ; Check that variable __llvm_fs_discriminator__ is generated.
 ; CHECK: .type   __llvm_fs_discriminator__,@object # @__llvm_fs_discriminator__
 ; CHECK: .section        .rodata,"a",@progbits
 ; CHECK: .weak   __llvm_fs_discriminator__
 ; CHECK: __llvm_fs_discriminator__:
 ; CHECK: .byte   1
 ; CHECK: .size   __llvm_fs_discriminator__, 1
 

llvm/test/CodeGen/X86/fsafdo_test2.ll

 ;;   int i;
 ;;   for (i = 0; i < 10000000; i++) {
 ;;     foo();
 ;;   }
 ;; }
 ;;
 ;; Check that fs-afdo discriminators are generated.
 ; CHECK: .loc    1 23 9 is_stmt 0 discriminator 1 # unroll.c:23:9
-; CHECK: .loc    1 23 9 is_stmt 0 discriminator 3892314113 # unroll.c:23:9
-; CHECK: .loc    1 23 9 is_stmt 0 discriminator 2818572289 # unroll.c:23:9
-; CHECK: .loc    1 23 9 is_stmt 0 discriminator 3623878657 # unroll.c:23:9
+; CHECK: .loc    1 23 9 is_stmt 0 discriminator 3585 # unroll.c:23:9
+; CHECK: .loc    1 23 9 is_stmt 0 discriminator 8705 # unroll.c:23:9
+; CHECK: .loc    1 23 9 is_stmt 0 discriminator 4097 # unroll.c:23:9
 ;;
 ;; Check that variable __llvm_fs_discriminator__ is generated.
 ; CHECK: .type   __llvm_fs_discriminator__,@object # @__llvm_fs_discriminator__
 ; CHECK: .section        .rodata,"a",@progbits
 ; CHECK: .weak   __llvm_fs_discriminator__
 ; CHECK: __llvm_fs_discriminator__:
 ; CHECK: .byte   1
 ; CHECK: .size   __llvm_fs_discriminator__, 1