[llvm] [DTLTO] [LLVM] Initial DTLTO cache implementation (PR #156433)

[Katya Romanova via llvm-commits]

================
@@ -2512,15 +2560,32 @@ class OutOfProcessThinBackend : public CGThinBackend {
             BCError + "cannot open native object file: " +
                 Job.NativeObjectPath + ": " + EC.message(),
             inconvertibleErrorCode());
-      auto StreamOrErr = AddStream(Job.Task, Job.ModuleID);
-      if (Error Err = StreamOrErr.takeError())
-        report_fatal_error(std::move(Err));
-      auto &Stream = *StreamOrErr->get();
-      *Stream.OS << ObjFileMbOrErr->get()->getMemBufferRef().getBuffer();
-      if (Error Err = Stream.commit())
-        report_fatal_error(std::move(Err));
-    }
 
+      MemoryBufferRef ObjFileMbRef = ObjFileMbOrErr->get()->getMemBufferRef();
+      if (Cache.isValid() && Job.CacheAddStream) {
----------------
romanova-ekaterina wrote:

Yes, if Job.CacheAddStream is non-null, Cache.isValid() is always true.

In this case I think it makes sense to leave Cache.IsValid as a part of if's condition for better understanding of the code, because there is an else statement below. 

Job.CacheAddStream is non-null, when we have a cache miss. However, if we get to this point, we cannot have a cache hit.  The only condition when we can execute "else" block below is when we don't have cache enabled, i.e. when Cache.isValid is false. 

If I remove  Cache.isValid() from the "if" condition the code will still be correct, but it won't be as readable. 

I propose to leave it "as is". What do you think?
 

https://github.com/llvm/llvm-project/pull/156433