Page MenuHomePhabricator

[clang][NewPM] Fix broken profile test
ClosedPublic

Authored by leonardchan on Jun 11 2019, 11:53 AM.

Details

Summary

This contains the part of D62225 which fixes Profile/gcc-flag-compatibility.c by adding the pass that allows default profile generation to work under the new PM. It seems that ./default.profraw was not being generated with new PM enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc accepted this revision.Jun 12 2019, 8:51 PM

Let's update the test to explicitly run w/ both PMs to make sure this keeps working. LGTM with that change.

This revision is now accepted and ready to land.Jun 12 2019, 8:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 10:23 AM
davidxl added a subscriber: xur.Jun 28 2019, 3:58 PM
davidxl added a subscriber: davidxl.

Rong, can you take a look at this patch?

xur added a comment.Jun 28 2019, 4:45 PM

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass
MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

I mean, I'm happy for the patch to be reverted, but I still really don't understand why this fixes the test to work *exactly* the same as w/ the old pass manager and doesn't break any other tests if it is completely wrong? It seems like there must be something strange with the test coverage if this is so far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can show a patch?

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

I mean, I'm happy for the patch to be reverted, but I still really don't understand why this fixes the test to work *exactly* the same as w/ the old pass manager and doesn't break any other tests if it is completely wrong? It seems like there must be something strange with the test coverage if this is so far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can show a patch?

This is also the fix I'm suggesting.

diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 5d66473e7b9..f924ecbd8c6 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -1220,12 +1220,13 @@ void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
       }
     }
 
-    if (CodeGenOpts.OptimizationLevel == 0)
+    if (CodeGenOpts.OptimizationLevel == 0) {
       addSanitizersAtO0(MPM, TargetTriple, LangOpts, CodeGenOpts);
 
-    if (CodeGenOpts.hasProfileIRInstr()) {
-      // This file is stored as the ProfileFile.
-      MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
+      if (CodeGenOpts.hasProfileIRInstr()) {
+        // This file is stored as the ProfileFile.
+        MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
+      }
     }
   }
In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

No, I think we should be doing PGO at O0 in the new PM if we do so in the old PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct to fix this test case. That seems plausible to me at least, and I think reverting and figuring out what the right way to do it is a fine approach.

I just think we also need to understand why *no other test failed*, and why the only test we have for doing PGO at O0 is this one and it could be "fixed" but such a deeply unrelated change....

xur added a comment.Jun 28 2019, 5:04 PM

they are not doing the exacly the same thing for old pass manager and new pass manger: old pass manger is doing instrumentation, while the new pass manager with this change is NOT.
the test is not check instrumentation, (it only check the variables that generates by InstroProfiling pass).
In this sense, the test is not well written.

I can draft a patch for this.

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

I mean, I'm happy for the patch to be reverted, but I still really don't understand why this fixes the test to work *exactly* the same as w/ the old pass manager and doesn't break any other tests if it is completely wrong? It seems like there must be something strange with the test coverage if this is so far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can show a patch?

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

No, I think we should be doing PGO at O0 in the new PM if we do so in the old PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct to fix this test case. That seems plausible to me at least, and I think reverting and figuring out what the right way to do it is a fine approach.

I just think we also need to understand why *no other test failed*, and why the only test we have for doing PGO at O0 is this one and it could be "fixed" but such a deeply unrelated change....

Understood. I'll revert this patch.

xur added a comment.Jun 28 2019, 5:06 PM
In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

No, I think we should be doing PGO at O0 in the new PM if we do so in the old PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct to fix this test case. That seems plausible to me at least, and I think reverting and figuring out what the right way to do it is a fine approach.

Just check the IR, we still not doing instrumentation at O0. This patch just mask the error.

I just think we also need to understand why *no other test failed*, and why the only test we have for doing PGO at O0 is this one and it could be "fixed" but such a deeply unrelated change....

We have special code to do PGO at O0 in old pass manager. This is not done in the new pass manager.

Reverted in r364692

In D63155#1563275, @xur wrote:

I just think we also need to understand why *no other test failed*, and why the only test we have for doing PGO at O0 is this one and it could be "fixed" but such a deeply unrelated change....

We have special code to do PGO at O0 in old pass manager. This is not done in the new pass manager.

I'm not sure how this addresses my question about test coverage.