From a6b407fbf5b4393ab19d5d53a8caa3bf0deffa58 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 23 Jan 2024 16:38:06 -0800 Subject: [PATCH 1/2] Convert AFDO mutators to TransitionMutator Convert afdoDepsMutator and afdoMutator to a TransitionMutator as a step towards variants-on-demand. Bug: 319288033 Test: afdo_test.go Change-Id: Ib05845455ccf43a07b3915a0d7b0a95896062f13 --- android/config.go | 8 +-- cc/afdo.go | 179 ++++++++++++++++++++-------------------------- cc/afdo_test.go | 4 +- cc/cc.go | 9 +-- rust/afdo.go | 4 +- 5 files changed, 87 insertions(+), 117 deletions(-) diff --git a/android/config.go b/android/config.go index eb1e647d4..a62e786c1 100644 --- a/android/config.go +++ b/android/config.go @@ -1462,18 +1462,18 @@ func (c *deviceConfig) PgoAdditionalProfileDirs() []string { } // AfdoProfile returns fully qualified path associated to the given module name -func (c *deviceConfig) AfdoProfile(name string) (*string, error) { +func (c *deviceConfig) AfdoProfile(name string) (string, error) { for _, afdoProfile := range c.config.productVariables.AfdoProfiles { split := strings.Split(afdoProfile, ":") if len(split) != 3 { - return nil, fmt.Errorf("AFDO_PROFILES has invalid value: %s. "+ + return "", fmt.Errorf("AFDO_PROFILES has invalid value: %s. "+ "The expected format is :", afdoProfile) } if split[0] == name { - return proptools.StringPtr(strings.Join([]string{split[1], split[2]}, ":")), nil + return strings.Join([]string{split[1], split[2]}, ":"), nil } } - return nil, nil + return "", nil } func (c *deviceConfig) VendorSepolicyDirs() []string { diff --git a/cc/afdo.go b/cc/afdo.go index 79fbae157..61550845d 100644 --- a/cc/afdo.go +++ b/cc/afdo.go @@ -27,23 +27,12 @@ import ( // This flag needs to be in both CFlags and LdFlags to ensure correct symbol ordering const afdoFlagsFormat = "-fprofile-sample-use=%s -fprofile-sample-accurate" -func recordMissingAfdoProfileFile(ctx android.BaseModuleContext, missing string) { - getNamedMapForConfig(ctx.Config(), modulesMissingProfileFileKey).Store(missing, true) -} - -type afdoRdep struct { - VariationName *string - ProfilePath *string -} - type AfdoProperties struct { // Afdo allows developers self-service enroll for // automatic feedback-directed optimization using profile data. Afdo bool FdoProfilePath *string `blueprint:"mutated"` - - AfdoRDeps []afdoRdep `blueprint:"mutated"` } type afdo struct { @@ -102,29 +91,6 @@ func (afdo *afdo) flags(ctx ModuleContext, flags Flags) Flags { return flags } -func (afdo *afdo) addDep(ctx BaseModuleContext, actx android.BottomUpMutatorContext) { - if ctx.Host() { - return - } - - if ctx.static() && !ctx.staticBinary() { - return - } - - if c, ok := ctx.Module().(*Module); ok && c.Enabled() { - if fdoProfileName, err := actx.DeviceConfig().AfdoProfile(actx.ModuleName()); fdoProfileName != nil && err == nil { - actx.AddFarVariationDependencies( - []blueprint.Variation{ - {Mutator: "arch", Variation: actx.Target().ArchVariation()}, - {Mutator: "os", Variation: "android"}, - }, - FdoProfileTag, - []string{*fdoProfileName}..., - ) - } - } -} - // FdoProfileMutator reads the FdoProfileProvider from a direct dep with FdoProfileTag // assigns FdoProfileInfo.Path to the FdoProfilePath mutated property func (c *Module) fdoProfileMutator(ctx android.BottomUpMutatorContext) { @@ -136,87 +102,96 @@ func (c *Module) fdoProfileMutator(ctx android.BottomUpMutatorContext) { return } - ctx.VisitDirectDepsWithTag(FdoProfileTag, func(m android.Module) { - if info, ok := android.OtherModuleProvider(ctx, m, FdoProfileProvider); ok { - c.afdo.Properties.FdoProfilePath = proptools.StringPtr(info.Path.String()) + if c.Host() { + return + } + + if c.static() && !c.staticBinary() { + return + } + + if c, ok := ctx.Module().(*Module); ok && c.Enabled() { + if fdoProfileName, err := ctx.DeviceConfig().AfdoProfile(ctx.ModuleName()); fdoProfileName != "" && err == nil { + deps := ctx.AddFarVariationDependencies( + []blueprint.Variation{ + {Mutator: "arch", Variation: ctx.Target().ArchVariation()}, + {Mutator: "os", Variation: "android"}, + }, + FdoProfileTag, + fdoProfileName) + if len(deps) > 0 && deps[0] != nil { + if info, ok := android.OtherModuleProvider(ctx, deps[0], FdoProfileProvider); ok { + c.afdo.Properties.FdoProfilePath = proptools.StringPtr(info.Path.String()) + } + } } - }) + } } var _ FdoProfileMutatorInterface = (*Module)(nil) -// Propagate afdo requirements down from binaries and shared libraries -func afdoDepsMutator(mctx android.TopDownMutatorContext) { - if m, ok := mctx.Module().(*Module); ok && m.afdo.afdoEnabled() { - path := m.afdo.Properties.FdoProfilePath - mctx.WalkDeps(func(dep android.Module, parent android.Module) bool { - tag := mctx.OtherModuleDependencyTag(dep) - libTag, isLibTag := tag.(libraryDependencyTag) - - // Do not recurse down non-static dependencies - if isLibTag { - if !libTag.static() { - return false - } - } else { - if tag != objDepTag && tag != reuseObjTag { - return false - } - } - - if dep, ok := dep.(*Module); ok { - dep.afdo.Properties.AfdoRDeps = append( - dep.afdo.Properties.AfdoRDeps, - afdoRdep{ - VariationName: proptools.StringPtr(encodeTarget(m.Name())), - ProfilePath: path, - }, - ) - } - - return true - }) +func afdoPropagateViaDepTag(tag blueprint.DependencyTag) bool { + libTag, isLibTag := tag.(libraryDependencyTag) + // Do not recurse down non-static dependencies + if isLibTag { + return libTag.static() + } else { + return tag == objDepTag || tag == reuseObjTag || tag == staticVariantTag } } -// Create afdo variants for modules that need them -func afdoMutator(mctx android.BottomUpMutatorContext) { - if m, ok := mctx.Module().(*Module); ok && m.afdo != nil { - if !m.static() && m.afdo.Properties.Afdo { - mctx.SetDependencyVariation(encodeTarget(m.Name())) - return +// afdoTransitionMutator creates afdo variants of cc modules. +type afdoTransitionMutator struct{} + +func (a *afdoTransitionMutator) Split(ctx android.BaseModuleContext) []string { + return []string{""} +} + +func (a *afdoTransitionMutator) OutgoingTransition(ctx android.OutgoingTransitionContext, sourceVariation string) string { + if m, ok := ctx.Module().(*Module); ok && m.afdo != nil { + if !afdoPropagateViaDepTag(ctx.DepTag()) { + return "" } - variationNames := []string{""} - - variantNameToProfilePath := make(map[string]*string) - - for _, afdoRDep := range m.afdo.Properties.AfdoRDeps { - variantName := *afdoRDep.VariationName - // An rdep can be set twice in AfdoRDeps because there can be - // more than one path from an afdo-enabled module to - // a static dep such as - // afdo_enabled_foo -> static_bar ----> static_baz - // \ ^ - // ----------------------| - // We only need to create one variant per unique rdep - if _, exists := variantNameToProfilePath[variantName]; !exists { - variationNames = append(variationNames, variantName) - variantNameToProfilePath[variantName] = afdoRDep.ProfilePath - } + if sourceVariation != "" { + return sourceVariation } - if len(variationNames) > 1 { - modules := mctx.CreateVariations(variationNames...) - for i, name := range variationNames { - if name == "" { - continue + if m.afdo.afdoEnabled() && !(m.static() && !m.staticBinary()) && !m.Host() { + return encodeTarget(ctx.Module().Name()) + } + } + return "" +} + +func (a *afdoTransitionMutator) IncomingTransition(ctx android.IncomingTransitionContext, incomingVariation string) string { + if m, ok := ctx.Module().(*Module); ok && m.afdo != nil { + return incomingVariation + } + return "" +} + +func (a *afdoTransitionMutator) Mutate(ctx android.BottomUpMutatorContext, variation string) { + if variation == "" { + return + } + + if m, ok := ctx.Module().(*Module); ok && m.afdo != nil { + m.Properties.PreventInstall = true + m.Properties.HideFromMake = true + m.afdo.Properties.Afdo = true + if fdoProfileName, err := ctx.DeviceConfig().AfdoProfile(decodeTarget(variation)); fdoProfileName != "" && err == nil { + deps := ctx.AddFarVariationDependencies( + []blueprint.Variation{ + {Mutator: "arch", Variation: ctx.Target().ArchVariation()}, + {Mutator: "os", Variation: "android"}, + }, + FdoProfileTag, + fdoProfileName) + if len(deps) > 0 && deps[0] != nil { + if info, ok := android.OtherModuleProvider(ctx, deps[0], FdoProfileProvider); ok { + m.afdo.Properties.FdoProfilePath = proptools.StringPtr(info.Path.String()) } - variation := modules[i].(*Module) - variation.Properties.PreventInstall = true - variation.Properties.HideFromMake = true - variation.afdo.Properties.Afdo = true - variation.afdo.Properties.FdoProfilePath = variantNameToProfilePath[name] } } } diff --git a/cc/afdo_test.go b/cc/afdo_test.go index b250ad1a1..584af0d6f 100644 --- a/cc/afdo_test.go +++ b/cc/afdo_test.go @@ -174,11 +174,11 @@ func TestAfdoEnabledOnStaticDepNoAfdo(t *testing.T) { libBar := result.ModuleForTests("libBar", "android_arm64_armv8-a_static").Module() if !hasDirectDep(result, libTest, libFoo.Module()) { - t.Errorf("libTest missing dependency on afdo variant of libFoo") + t.Errorf("libTest missing dependency on non-afdo variant of libFoo") } if !hasDirectDep(result, libFoo.Module(), libBar) { - t.Errorf("libFoo missing dependency on afdo variant of libBar") + t.Errorf("libFoo missing dependency on non-afdo variant of libBar") } fooVariants := result.ModuleVariantsForTests("foo") diff --git a/cc/cc.go b/cc/cc.go index 1a9c7bf66..cac52cf50 100644 --- a/cc/cc.go +++ b/cc/cc.go @@ -55,7 +55,7 @@ func RegisterCCBuildComponents(ctx android.RegistrationContext) { ctx.BottomUp("test_per_src", TestPerSrcMutator).Parallel() ctx.BottomUp("version", versionMutator).Parallel() ctx.BottomUp("begin", BeginMutator).Parallel() - ctx.BottomUp("fdo_profile", fdoProfileMutator) + ctx.BottomUp("fdo_profile", fdoProfileMutator).Parallel() }) ctx.PostDepsMutators(func(ctx android.RegisterMutatorsContext) { @@ -70,8 +70,7 @@ func RegisterCCBuildComponents(ctx android.RegistrationContext) { ctx.Transition("coverage", &coverageTransitionMutator{}) - ctx.TopDown("afdo_deps", afdoDepsMutator) - ctx.BottomUp("afdo", afdoMutator).Parallel() + ctx.Transition("afdo", &afdoTransitionMutator{}) ctx.Transition("orderfile", &orderfileTransitionMutator{}) @@ -2339,10 +2338,6 @@ func (c *Module) beginMutator(actx android.BottomUpMutatorContext) { } ctx.ctx = ctx - if !actx.Host() || !ctx.static() || ctx.staticBinary() { - c.afdo.addDep(ctx, actx) - } - c.begin(ctx) } diff --git a/rust/afdo.go b/rust/afdo.go index 323ee36a5..6116c5e21 100644 --- a/rust/afdo.go +++ b/rust/afdo.go @@ -44,14 +44,14 @@ func (afdo *afdo) addDep(ctx BaseModuleContext, actx android.BottomUpMutatorCont if err != nil { ctx.ModuleErrorf("%s", err.Error()) } - if fdoProfileName != nil { + if fdoProfileName != "" { actx.AddFarVariationDependencies( []blueprint.Variation{ {Mutator: "arch", Variation: actx.Target().ArchVariation()}, {Mutator: "os", Variation: "android"}, }, cc.FdoProfileTag, - []string{*fdoProfileName}..., + []string{fdoProfileName}..., ) } } From 943aa5c862dfd2da994c8f44006767be518919ab Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 24 Jan 2024 14:44:47 -0800 Subject: [PATCH 2/2] Remove fdoProfileMutator Add the dependency on the fdo_profile module in afdoTransitionMutator and read the provider in GenerateAndroidBuildActions. Bug: 319288033 Test: afdo_test.go Change-Id: Ied8fd7b52d5694a3691652318e87b8fe14dda126 --- cc/afdo.go | 102 ++++++++++++++++------------------------------ cc/cc.go | 3 +- cc/fdo_profile.go | 25 +----------- 3 files changed, 37 insertions(+), 93 deletions(-) diff --git a/cc/afdo.go b/cc/afdo.go index 61550845d..0de1d4ba6 100644 --- a/cc/afdo.go +++ b/cc/afdo.go @@ -21,7 +21,6 @@ import ( "android/soong/android" "github.com/google/blueprint" - "github.com/google/blueprint/proptools" ) // This flag needs to be in both CFlags and LdFlags to ensure correct symbol ordering @@ -32,7 +31,7 @@ type AfdoProperties struct { // automatic feedback-directed optimization using profile data. Afdo bool - FdoProfilePath *string `blueprint:"mutated"` + AfdoDep bool `blueprint:"mutated"` } type afdo struct { @@ -57,7 +56,7 @@ func (afdo *afdo) afdoEnabled() bool { } func (afdo *afdo) flags(ctx ModuleContext, flags Flags) Flags { - if afdo.Properties.Afdo { + if afdo.Properties.Afdo || afdo.Properties.AfdoDep { // We use `-funique-internal-linkage-names` to associate profiles to the right internal // functions. This option should be used before generating a profile. Because a profile // generated for a binary without unique names doesn't work well building a binary with @@ -75,61 +74,39 @@ func (afdo *afdo) flags(ctx ModuleContext, flags Flags) Flags { // TODO(b/266595187): Remove the following feature once it is enabled in LLVM by default. flags.Local.CFlags = append([]string{"-mllvm", "-improved-fs-discriminator=true"}, flags.Local.CFlags...) } - if path := afdo.Properties.FdoProfilePath; path != nil { - // The flags are prepended to allow overriding. - profileUseFlag := fmt.Sprintf(afdoFlagsFormat, *path) - flags.Local.CFlags = append([]string{profileUseFlag}, flags.Local.CFlags...) - flags.Local.LdFlags = append([]string{profileUseFlag, "-Wl,-mllvm,-no-warn-sample-unused=true"}, flags.Local.LdFlags...) + fdoProfileDeps := ctx.GetDirectDepsWithTag(FdoProfileTag) + if len(fdoProfileDeps) > 0 && fdoProfileDeps[0] != nil { + if info, ok := android.OtherModuleProvider(ctx, fdoProfileDeps[0], FdoProfileProvider); ok { + fdoProfilePath := info.Path.String() - // Update CFlagsDeps and LdFlagsDeps so the module is rebuilt - // if profileFile gets updated - pathForSrc := android.PathForSource(ctx, *path) - flags.CFlagsDeps = append(flags.CFlagsDeps, pathForSrc) - flags.LdFlagsDeps = append(flags.LdFlagsDeps, pathForSrc) + // The flags are prepended to allow overriding. + profileUseFlag := fmt.Sprintf(afdoFlagsFormat, fdoProfilePath) + flags.Local.CFlags = append([]string{profileUseFlag}, flags.Local.CFlags...) + flags.Local.LdFlags = append([]string{profileUseFlag, "-Wl,-mllvm,-no-warn-sample-unused=true"}, flags.Local.LdFlags...) + + // Update CFlagsDeps and LdFlagsDeps so the module is rebuilt + // if profileFile gets updated + pathForSrc := android.PathForSource(ctx, fdoProfilePath) + flags.CFlagsDeps = append(flags.CFlagsDeps, pathForSrc) + flags.LdFlagsDeps = append(flags.LdFlagsDeps, pathForSrc) + } } return flags } -// FdoProfileMutator reads the FdoProfileProvider from a direct dep with FdoProfileTag -// assigns FdoProfileInfo.Path to the FdoProfilePath mutated property -func (c *Module) fdoProfileMutator(ctx android.BottomUpMutatorContext) { - if !c.Enabled() { - return - } - - if !c.afdo.afdoEnabled() { - return - } - - if c.Host() { - return - } - - if c.static() && !c.staticBinary() { - return - } - - if c, ok := ctx.Module().(*Module); ok && c.Enabled() { - if fdoProfileName, err := ctx.DeviceConfig().AfdoProfile(ctx.ModuleName()); fdoProfileName != "" && err == nil { - deps := ctx.AddFarVariationDependencies( - []blueprint.Variation{ - {Mutator: "arch", Variation: ctx.Target().ArchVariation()}, - {Mutator: "os", Variation: "android"}, - }, - FdoProfileTag, - fdoProfileName) - if len(deps) > 0 && deps[0] != nil { - if info, ok := android.OtherModuleProvider(ctx, deps[0], FdoProfileProvider); ok { - c.afdo.Properties.FdoProfilePath = proptools.StringPtr(info.Path.String()) - } - } - } +func (a *afdo) addDep(ctx android.BottomUpMutatorContext, fdoProfileTarget string) { + if fdoProfileName, err := ctx.DeviceConfig().AfdoProfile(fdoProfileTarget); fdoProfileName != "" && err == nil { + ctx.AddFarVariationDependencies( + []blueprint.Variation{ + {Mutator: "arch", Variation: ctx.Target().ArchVariation()}, + {Mutator: "os", Variation: "android"}, + }, + FdoProfileTag, + fdoProfileName) } } -var _ FdoProfileMutatorInterface = (*Module)(nil) - func afdoPropagateViaDepTag(tag blueprint.DependencyTag) bool { libTag, isLibTag := tag.(libraryDependencyTag) // Do not recurse down non-static dependencies @@ -172,27 +149,16 @@ func (a *afdoTransitionMutator) IncomingTransition(ctx android.IncomingTransitio } func (a *afdoTransitionMutator) Mutate(ctx android.BottomUpMutatorContext, variation string) { - if variation == "" { - return - } - if m, ok := ctx.Module().(*Module); ok && m.afdo != nil { - m.Properties.PreventInstall = true - m.Properties.HideFromMake = true - m.afdo.Properties.Afdo = true - if fdoProfileName, err := ctx.DeviceConfig().AfdoProfile(decodeTarget(variation)); fdoProfileName != "" && err == nil { - deps := ctx.AddFarVariationDependencies( - []blueprint.Variation{ - {Mutator: "arch", Variation: ctx.Target().ArchVariation()}, - {Mutator: "os", Variation: "android"}, - }, - FdoProfileTag, - fdoProfileName) - if len(deps) > 0 && deps[0] != nil { - if info, ok := android.OtherModuleProvider(ctx, deps[0], FdoProfileProvider); ok { - m.afdo.Properties.FdoProfilePath = proptools.StringPtr(info.Path.String()) - } + if variation == "" { + if m.afdo.afdoEnabled() && !(m.static() && !m.staticBinary()) && !m.Host() { + m.afdo.addDep(ctx, ctx.ModuleName()) } + } else { + m.Properties.PreventInstall = true + m.Properties.HideFromMake = true + m.afdo.Properties.AfdoDep = true + m.afdo.addDep(ctx, decodeTarget(variation)) } } } diff --git a/cc/cc.go b/cc/cc.go index cac52cf50..480d3141e 100644 --- a/cc/cc.go +++ b/cc/cc.go @@ -55,7 +55,6 @@ func RegisterCCBuildComponents(ctx android.RegistrationContext) { ctx.BottomUp("test_per_src", TestPerSrcMutator).Parallel() ctx.BottomUp("version", versionMutator).Parallel() ctx.BottomUp("begin", BeginMutator).Parallel() - ctx.BottomUp("fdo_profile", fdoProfileMutator).Parallel() }) ctx.PostDepsMutators(func(ctx android.RegisterMutatorsContext) { @@ -1382,7 +1381,7 @@ func (c *Module) IsVndk() bool { func (c *Module) isAfdoCompile() bool { if afdo := c.afdo; afdo != nil { - return afdo.Properties.FdoProfilePath != nil + return afdo.Properties.Afdo || afdo.Properties.AfdoDep } return false } diff --git a/cc/fdo_profile.go b/cc/fdo_profile.go index 0893da5e0..1a3395773 100644 --- a/cc/fdo_profile.go +++ b/cc/fdo_profile.go @@ -43,23 +43,10 @@ type FdoProfileInfo struct { } // FdoProfileProvider is used to provide path to an fdo profile -var FdoProfileProvider = blueprint.NewMutatorProvider[FdoProfileInfo]("fdo_profile") - -// FdoProfileMutatorInterface is the interface implemented by fdo_profile module type -// module types that can depend on an fdo_profile module -type FdoProfileMutatorInterface interface { - // FdoProfileMutator eithers set or get FdoProfileProvider - fdoProfileMutator(ctx android.BottomUpMutatorContext) -} - -var _ FdoProfileMutatorInterface = (*fdoProfile)(nil) +var FdoProfileProvider = blueprint.NewProvider[FdoProfileInfo]() // GenerateAndroidBuildActions of fdo_profile does not have any build actions -func (fp *fdoProfile) GenerateAndroidBuildActions(ctx android.ModuleContext) {} - -// FdoProfileMutator sets FdoProfileProvider to fdo_profile module -// or sets afdo.Properties.FdoProfilePath to path in FdoProfileProvider of the depended fdo_profile -func (fp *fdoProfile) fdoProfileMutator(ctx android.BottomUpMutatorContext) { +func (fp *fdoProfile) GenerateAndroidBuildActions(ctx android.ModuleContext) { if fp.properties.Profile != nil { path := android.PathForModuleSrc(ctx, *fp.properties.Profile) android.SetProvider(ctx, FdoProfileProvider, FdoProfileInfo{ @@ -68,14 +55,6 @@ func (fp *fdoProfile) fdoProfileMutator(ctx android.BottomUpMutatorContext) { } } -// fdoProfileMutator calls the generic fdoProfileMutator function of fdoProfileMutator -// which is implemented by cc and cc.FdoProfile -func fdoProfileMutator(ctx android.BottomUpMutatorContext) { - if f, ok := ctx.Module().(FdoProfileMutatorInterface); ok { - f.fdoProfileMutator(ctx) - } -} - func FdoProfileFactory() android.Module { m := &fdoProfile{} m.AddProperties(&m.properties)