On Mon, Mar 2, 2015 at 1:15 PM, Ahmed Bougacha <[email protected]> wrote: > On Mon, Mar 2, 2015 at 12:38 PM, Duncan P. N. Exon Smith > <[email protected]> wrote: >> +echristo >> >>> On 2015-Mar-02, at 09:55, Ahmed Bougacha <[email protected]> wrote: >>> >>> - Only emit flag when GlobalMerge is disabled (per Akira and Duncan's >>> reviews) >>> >>> I added a target check to only emit the flags on ARM/AArch64. I'm fine >>> with emitting the flag no matter the target, if desired (less brittle), but >>> I personally don't like adding noise to every module ever, hence hiding it. >> >> Now the logic is what I expected, but I'm less confident that it's the >> best path. >> >> The logic shakes out like this: >> >> - Change the driver to *always* specify -m{no-,}global-merge. >> - In -cc1: >> - If -mglobal-merge: don't set any flags. >> - If -mno-global-merge: add `Error` flag to explicitly disable it, >> if this is a relevant target. >> - When linking modules: >> - If any module has the flag, it's disabled in the linked module. >> - When running codegen: run global-merge (if possible) unless it has >> been disabled. >> >> Maybe I haven't had enough sleep, but this other logic seems more >> appealing to me now. >> >> - Use the old driver logic (pass the option through only if >> specified). > > I believe this is already the case with the patch, no? Pass the > option (-m or -mno) only if specified (if getLastArg found something). > >> - In -cc1: >> - -mno-global-merge: add `Override` flag to disable it. >> - -mglobal-merge: add `Error` flag to enable it. >> - (Otherwise, set the flag based on optimization level? Or don't >> set a flag?) > > Note that in real life (tm), the flag isn't passed that often (if > ever: it used to be broken for a while, until someone eventually > noticed). > The interesting case is deciding whether to enable it or not in LTO. > If you compile with -O1/-Os, you need a way to tell the LTO backend to > respect that; it always uses -O3 (a generic "original optimization > level" doesn't really work, because you might compile files with > different levels, and I'm not sure how you can merge that). > That's why I want to set a flag based on the clang optimization level. > Does that make sense? > >> - When linking modules: >> - If any module disabled it, it's disabled in the linked module. >> - Else, if any module has enabled it, it's enabled in the linked >> module. >> - Else, the linked module has neither flag. > > To be clear, this has the same behavior as the current patch, if on > the LLVM side the pass is enabled by default when the flag is missing, > right? > > The logic SGTM, the 'Override' seems cleaner indeed. > >> - When running codegen: respect the flag if present; otherwise, use >> old logic (optimization level?). > > Yep, and yep (-O1 backend level). > >> @Eric/Akira/Ahmed, any thoughts? >> >> @Ahmed, if we stick with this logic, I have some nitpicks below on the >> spelling. >> >> Also, was there an LLVM patch that went with this? Can you attach it to >> the same thread? > > Yep, separate under http://reviews.llvm.org/D7969. Let's discuss > here, I'll update that one when we come up with something. > >> >>> Index: lib/CodeGen/CodeGenModule.cpp >>> =================================================================== >>> --- lib/CodeGen/CodeGenModule.cpp >>> +++ lib/CodeGen/CodeGenModule.cpp >>> @@ -418,6 +418,8 @@ >>> EmitVersionIdentMetadata(); >>> >>> EmitTargetMetadata(); >>> + >>> + EmitBackendOptionsMetadata(); >>> } >>> >>> void CodeGenModule::UpdateCompletedType(const TagDecl *TD) { >>> @@ -3609,6 +3611,18 @@ >>> } >>> } >>> >>> +void CodeGenModule::EmitBackendOptionsMetadata() { >>> + // Only emit the "Global Merge" flag when it disables it, and only on >>> targets >>> + // that support it (ARM/AArch64). >>> + llvm::Triple::ArchType Arch = >>> Context.getTargetInfo().getTriple().getArch(); >>> + if ((Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb || >>> + Arch == llvm::Triple::thumb || Arch == llvm::Triple::thumbeb || >>> + Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be) >>> && >>> + getCodeGenOpts().NoGlobalMerge) >>> + getModule().addModuleFlag(llvm::Module::Error, "Enable Global Merge", >>> + llvm::MDString::get(VMContext, "false")); >> >> I think this should be: >> >> getModule().addModuleFlag(llvm::Module::Error, "Disable Global Merge", >> 1u); > > Akira also proposed "disable" ;) I'll change that and the > string->unsigned, then!
We discussed this a bit more, and we agreed on a "Global Merge" flag, following Duncan's logic: the disabled case (either -mno-global-merge or -O < 3) adds an "Override" flag, vs. "Error" for the enabled case. The default is to enable it, when the flag isn't present, to match the current behavior. Eric, thoughts? -Ahmed _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
