http://reviews.llvm.org/D16250
On Fri, Jan 15, 2016 at 5:35 PM, Justin Lebar <jle...@google.com> wrote: > If you revert this you'll have to revert three other dependent > patches. I'm looking at a fix, if you're able to sit tight for > fifteen minutes or so. > > If not, I can at least contribute a testcase; that much is done. > > On Fri, Jan 15, 2016 at 5:33 PM, Eric Christopher <echri...@gmail.com> wrote: >> I'm afk at the moment please revert and add a test case if you can? >> >> >> On Fri, Jan 15, 2016, 5:24 PM Chris Bieneman <be...@apple.com> wrote: >>> >>> +Eric, >>> >>> If this patch can’t be quickly fixed, can it please be reverted? >>> >>> -Chris >>> >>> On Jan 15, 2016, at 4:36 PM, Chris Bieneman via cfe-commits >>> <cfe-commits@lists.llvm.org> wrote: >>> >>> This change breaks darwin fat binary support. >>> >>> > ./bin/clang++ ~/dev/scratch/hello_world.cpp -arch armv7 -arch armv7s >>> > -isysroot >>> > /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/ >>> fatal error: >>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: >>> /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out and >>> /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out have >>> the same architectures (armv7) and can't be in the same fat output file >>> clang-3.9: error: lipo command failed with exit code 1 (use -v to see >>> invocation) >>> >>> Can someone more familiar with this code please take a look? >>> >>> Thanks, >>> >>> -Chris >>> >>> On Jan 14, 2016, at 1:41 PM, Justin Lebar via cfe-commits >>> <cfe-commits@lists.llvm.org> wrote: >>> >>> Author: jlebar >>> Date: Thu Jan 14 15:41:21 2016 >>> New Revision: 257808 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=257808&view=rev >>> Log: >>> Don't build jobs for the same Action + ToolChain twice. >>> >>> Summary: >>> Right now if the Action graph is a DAG and we encounter an action twice, >>> we will run it twice. >>> >>> This patch is difficult to test as-is, but I have testcases for this as >>> used within CUDA compilation. >>> >>> Reviewers: >>> >>> Subscribers: >>> >>> Modified: >>> cfe/trunk/include/clang/Driver/Driver.h >>> cfe/trunk/lib/Driver/Driver.cpp >>> >>> Modified: cfe/trunk/include/clang/Driver/Driver.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Driver.h?rev=257808&r1=257807&r2=257808&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Driver/Driver.h (original) >>> +++ cfe/trunk/include/clang/Driver/Driver.h Thu Jan 14 15:41:21 2016 >>> @@ -21,6 +21,7 @@ >>> #include "llvm/Support/Path.h" // FIXME: Kill when CompilationInfo lands. >>> >>> #include <list> >>> +#include <map> >>> #include <memory> >>> #include <set> >>> #include <string> >>> @@ -381,10 +382,13 @@ public: >>> >>> /// BuildJobsForAction - Construct the jobs to perform for the >>> /// action \p A and return an InputInfo for the result of running \p A. >>> + /// Will only construct jobs for a given (Action, ToolChain) pair once. >>> InputInfo BuildJobsForAction(Compilation &C, const Action *A, >>> const ToolChain *TC, const char *BoundArch, >>> bool AtTopLevel, bool MultipleArchs, >>> - const char *LinkingOutput) const; >>> + const char *LinkingOutput, >>> + std::map<std::pair<const Action *, >>> std::string>, >>> + InputInfo> &CachedResults) const; >>> >>> /// Returns the default name for linked images (e.g., "a.out"). >>> const char *getDefaultImageName() const; >>> @@ -441,6 +445,16 @@ private: >>> /// the driver mode. >>> std::pair<unsigned, unsigned> getIncludeExcludeOptionFlagMasks() const; >>> >>> + /// Helper used in BuildJobsForAction. Doesn't use the cache when >>> building >>> + /// jobs specifically for the given action, but will use the cache when >>> + /// building jobs for the Action's inputs. >>> + InputInfo BuildJobsForActionNoCache( >>> + Compilation &C, const Action *A, const ToolChain *TC, >>> + const char *BoundArch, bool AtTopLevel, bool MultipleArchs, >>> + const char *LinkingOutput, >>> + std::map<std::pair<const Action *, std::string>, InputInfo> >>> + &CachedResults) const; >>> + >>> public: >>> /// GetReleaseVersion - Parse (([0-9]+)(.([0-9]+)(.([0-9]+)?))?)? and >>> /// return the grouped values as integers. Numbers which are not >>> >>> Modified: cfe/trunk/lib/Driver/Driver.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=257808&r1=257807&r2=257808&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Driver/Driver.cpp (original) >>> +++ cfe/trunk/lib/Driver/Driver.cpp Thu Jan 14 15:41:21 2016 >>> @@ -1632,6 +1632,8 @@ void Driver::BuildJobs(Compilation &C) c >>> if (A->getOption().matches(options::OPT_arch)) >>> ArchNames.insert(A->getValue()); >>> >>> + // Set of (Action, canonical ToolChain triple) pairs we've built jobs >>> for. >>> + std::map<std::pair<const Action *, std::string>, InputInfo> >>> CachedResults; >>> for (Action *A : C.getActions()) { >>> // If we are linking an image for multiple archs then the linker wants >>> // -arch_multiple and -final_output <final image name>. Unfortunately, >>> this >>> @@ -1651,7 +1653,7 @@ void Driver::BuildJobs(Compilation &C) c >>> /*BoundArch*/ nullptr, >>> /*AtTopLevel*/ true, >>> /*MultipleArchs*/ ArchNames.size() > 1, >>> - /*LinkingOutput*/ LinkingOutput); >>> + /*LinkingOutput*/ LinkingOutput, CachedResults); >>> } >>> >>> // If the user passed -Qunused-arguments or there were errors, don't >>> warn >>> @@ -1779,19 +1781,38 @@ static const Tool *selectToolForJob(Comp >>> return ToolForJob; >>> } >>> >>> -InputInfo Driver::BuildJobsForAction(Compilation &C, const Action *A, >>> - const ToolChain *TC, const char >>> *BoundArch, >>> - bool AtTopLevel, bool MultipleArchs, >>> - const char *LinkingOutput) const { >>> +InputInfo Driver::BuildJobsForAction( >>> + Compilation &C, const Action *A, const ToolChain *TC, const char >>> *BoundArch, >>> + bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput, >>> + std::map<std::pair<const Action *, std::string>, InputInfo> >>> &CachedResults) >>> + const { >>> + std::pair<const Action *, std::string> ActionTC = { >>> + A, TC->getTriple().normalize()}; >>> + auto CachedResult = CachedResults.find(ActionTC); >>> + if (CachedResult != CachedResults.end()) { >>> + return CachedResult->second; >>> + } >>> + InputInfo Result = >>> + BuildJobsForActionNoCache(C, A, TC, BoundArch, AtTopLevel, >>> MultipleArchs, >>> + LinkingOutput, CachedResults); >>> + CachedResults[ActionTC] = Result; >>> + return Result; >>> +} >>> + >>> +InputInfo Driver::BuildJobsForActionNoCache( >>> + Compilation &C, const Action *A, const ToolChain *TC, const char >>> *BoundArch, >>> + bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput, >>> + std::map<std::pair<const Action *, std::string>, InputInfo> >>> &CachedResults) >>> + const { >>> llvm::PrettyStackTraceString CrashInfo("Building compilation jobs"); >>> >>> InputInfoList CudaDeviceInputInfos; >>> if (const CudaHostAction *CHA = dyn_cast<CudaHostAction>(A)) { >>> // Append outputs of device jobs to the input list. >>> for (const Action *DA : CHA->getDeviceActions()) { >>> - CudaDeviceInputInfos.push_back( >>> - BuildJobsForAction(C, DA, TC, nullptr, AtTopLevel, >>> - /*MultipleArchs*/ false, LinkingOutput)); >>> + CudaDeviceInputInfos.push_back(BuildJobsForAction( >>> + C, DA, TC, nullptr, AtTopLevel, >>> + /*MultipleArchs*/ false, LinkingOutput, CachedResults)); >>> } >>> // Override current action with a real host compile action and >>> continue >>> // processing it. >>> @@ -1822,7 +1843,7 @@ InputInfo Driver::BuildJobsForAction(Com >>> TC = &C.getDefaultToolChain(); >>> >>> return BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel, >>> - MultipleArchs, LinkingOutput); >>> + MultipleArchs, LinkingOutput, >>> CachedResults); >>> } >>> >>> if (const CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) { >>> @@ -1831,7 +1852,8 @@ InputInfo Driver::BuildJobsForAction(Com >>> assert(CDA->getGpuArchName() && "No GPU name in device action."); >>> return BuildJobsForAction(C, *CDA->begin(), >>> C.getCudaDeviceToolChain(), >>> CDA->getGpuArchName(), CDA->isAtTopLevel(), >>> - /*MultipleArchs*/ true, LinkingOutput); >>> + /*MultipleArchs*/ true, LinkingOutput, >>> + CachedResults); >>> } >>> >>> const ActionList *Inputs = &A->getInputs(); >>> @@ -1847,9 +1869,9 @@ InputInfo Driver::BuildJobsForAction(Com >>> // need to build jobs for device-side inputs it may have held. >>> if (CollapsedCHA) { >>> for (const Action *DA : CollapsedCHA->getDeviceActions()) { >>> - CudaDeviceInputInfos.push_back( >>> - BuildJobsForAction(C, DA, TC, "", AtTopLevel, >>> - /*MultipleArchs*/ false, LinkingOutput)); >>> + CudaDeviceInputInfos.push_back(BuildJobsForAction( >>> + C, DA, TC, "", AtTopLevel, >>> + /*MultipleArchs*/ false, LinkingOutput, CachedResults)); >>> } >>> } >>> >>> @@ -1863,7 +1885,7 @@ InputInfo Driver::BuildJobsForAction(Com >>> AtTopLevel && (isa<DsymutilJobAction>(A) || >>> isa<VerifyJobAction>(A)); >>> InputInfos.push_back(BuildJobsForAction(C, Input, TC, BoundArch, >>> SubJobAtTopLevel, >>> MultipleArchs, >>> - LinkingOutput)); >>> + LinkingOutput, >>> CachedResults)); >>> } >>> >>> // Always use the first input as the base input. >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >>> >> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
