Optimization record for bytecode input missing- PR44000

[zahiraam]

Summary

Optimization record is not generated when the input file is in bytecode format.

[craig.topper]

clang/lib/CodeGen/CodeGenAction.cpp

85

retruns -> returns

313

You already returned if isSet is false so this check is uneeded.

1120

I don't work in the frontend. Can setASTConsumer fail somehow or would hasASTConsumer always return true.

1123

This looks strangely formatted. The opening parenthese should be on the previous line. Please run clang-format on this

llvm/include/llvm/IR/LLVMContext.h

151 ↗

(On Diff #230941)

Bite->Bit

[craig.topper]

llvm/lib/IR/LLVMContext.cpp

118 ↗

(On Diff #230941)

Why do we need a separate entry point that does the same thing as setDiagnosticHandler?

[zahiraam]

clang/lib/CodeGen/CodeGenAction.cpp

1120

setASTConsumer can return a nullptr.

llvm/lib/IR/LLVMContext.cpp

118 ↗

(On Diff #230941)

setDiagnosticHandler calls builds an object of class ClangDiagnosticHandler that is a base class for diagnostic handling in LLVM.
The handleDiagnostics method must be overriden by the subclasses to handle the diagnistic. For bitecode input, the derived class
doesn't exist. So it needs to be created.

[craig.topper]

clang/lib/CodeGen/CodeGenAction.cpp

1123

This also needs to be bitcode instead of bitecode

llvm/lib/IR/LLVMContext.cpp

118 ↗

(On Diff #230941)

I don’t understand. No objects are created here. The object is created by clang and passed here. This function is identical to the one at line 132.

[zahiraam]

After an (offline) review from @erichkeane (added now as a reviewer) , stripped the unnecessary code. Thanks.

[erichkeane]

unique_ptr already has a 'bool' builtin to its state (one that this code checks later anyway), so there isn't a good reason to return a bool here. Otherwise I think I'm OK with this.

I'd like @thegameg to have a chance to take a look to make sure everything this is doing is necessary, so you should perhaps hold off until ~Weds/Thurs before committing.

clang/lib/CodeGen/CodeGenAction.cpp

87

This should just make/return OptRecordFile rather than a bool.

[thegameg]

Thanks for fixing this!

clang/lib/CodeGen/CodeGenAction.cpp

97

I think what would be nicer here is to extract all the error handling in a separate function and keep using setupOptimizationRemarks for all the callers.

305

This would become something like:

Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
      llvm::setupOptimizationRemarks(...);
if (Error E = OptRecordFileOrErr.takeError()) {
  reportError(std::move(E));
  return;
}

clang/test/CodeGen/opt-record-1.c

3

Why are the two -check-prefixes needed?

4

Extra blank line here.

18

Extra blank line here too.

[zahiraam]

Thanks for the review. Please let me know if I have addressed everything.

clang/test/CodeGen/opt-record-1.c

3

Removed one.

[erichkeane]

Otherwise I think I'm OK. @thegameg ?

clang/lib/CodeGen/CodeGenAction.cpp

85

not sure we can call this "reportError", as its too generic. how about reportOptRecordError?

[zahiraam]

Changed it. Thanks.

[thegameg]

LGTM with one last nit, thanks!

clang/lib/CodeGen/CodeGenAction.cpp

1084

Indentation looks a bit off here.

[zahiraam]

Fixed the indentation. Thanks.

[leonardchan]

Hi! I believe the commit for this (a3b2552575d3c333e928446fac10cc5b0b4092a9) is causing a test failure on our bots (https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8894791990609720880) and some buildbots (can't find the link since it scrolled past the end of the console). Could you look into it when you get a chance?

FAIL: Clang :: CodeGen/opt-record-1.c (2905 of 16336)
******************** TEST 'Clang :: CodeGen/opt-record-1.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc /b/s/w/ir/k/llvm-project/clang/test/CodeGen/opt-record-1.c -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/tools/clang/test/CodeGen/Output/opt-record-1.c.tmp.bc
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/lib/clang/10.0.0/include -nostdsysteminc -x ir /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/tools/clang/test/CodeGen/Output/opt-record-1.c.tmp.bc -opt-record-file /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/tools/clang/test/CodeGen/Output/opt-record-1.c.tmp.opt -fopenmp -emit-obj
: 'RUN: at line 3';   cat /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/tools/clang/test/CodeGen/Output/opt-record-1.c.tmp.opt |  /b/s/w/ir/k/recipe_cleanup/clang6Bs5au/llvm_build_dir/bin/FileCheck -check-prefix=CHECK  /b/s/w/ir/k/llvm-project/clang/test/CodeGen/opt-record-1.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/CodeGen/opt-record-1.c:12:11: error: CHECK: expected string not found in input
// CHECK: --- !Missed
          ^
<stdin>:1:1: note: scanning from here
--- !Analysis
^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 

Testing Time: 74.62s
********************
Failing Tests (1):
    Clang :: CodeGen/opt-record-1.c

[zahiraam]

Please see the edit in the list test. Fixed the build issue that occurred in the buildbot:
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/232/steps/test-check-all/logs/stdio

[zahiraam]

If you agree with the edit I will commit the change. Thanks.

[zahiraam]

This might be easier to read the edit that I have made:
ksh-3.2$ git diff
diff --git a/clang/test/CodeGen/opt-record-1.c b/clang/test/CodeGen/opt-record-1.c
index 3f37e32..00a753d 100644

• a/clang/test/CodeGen/opt-record-1.c

+++ b/clang/test/CodeGen/opt-record-1.c
@@ -1,12 +1,12 @@
- RUN: %clang_cc1 %s -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
- RUN: %clang_cc1 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 %s -O3 -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -O3 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
// RUN: cat %t.opt | FileCheck -check-prefix=CHECK %s

void foo(int *a, int *b, int *c) {
#pragma omp parallel for

• for (int i = 0; i < 100; i++) {
• a[i] = b[i] + c[i];
• }

+ for (int i = 0; i < 100; i++) {
+ a[i] = b[i] + c[i];
+ }
}

// CHECK: --- !Missed
ksh-3.2$

And I checked that it passes.

[thegameg]

LGTM

[leonardchan]

LGTM. Could you commit this soon to get the bots green again?

[zahiraam]

In D70691#1773611, @leonardchan wrote:

LGTM. Could you commit this soon to get the bots green again?

@leonardchan The fix was committed. Thanks.

[zahiraam]

Got other build fails.
Doesn't the LIT test require a:
// REQUIRES: x86-registered-target
command?

Thanks.

[erichkeane]

I looked at the test failure, the REQUIRES line is necessary because the test calls emit-obj. I've asked Zahira to commit it to fix the bots.

[zahiraam]

In D70691#1775169, @erichkeane wrote:

I looked at the test failure, the REQUIRES line is necessary because the test calls emit-obj. I've asked Zahira to commit it to fix the bots.

Done. Thanks Erich.

clang/lib/CodeGen/CodeGenAction.cpp

bool isAnyRemarkEnabled() const override {
               CodeGenOpts.OptimizationRemarkPattern);
     }
 
   private:
     const CodeGenOptions &CodeGenOpts;
     BackendConsumer *BackendCon;
   };
 
+  // setOptRecordFile returns true if there is a record file. The record file can
+  // be an empty file. If it returns false, then there is an error.
+  static bool setOptRecordFile(const CodeGenOptions CGOpts, DiagnosticsEngine &Diags,
+                               LLVMContext &Ctx,
+                               std::unique_ptr<llvm::ToolOutputFile> &OptRecordFile)
+  {
+    Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
+      llvm::setupOptimizationRemarks(Ctx, CGOpts.OptRecordFile,
+        CGOpts.OptRecordPasses,
+        CGOpts.OptRecordFormat,
+        CGOpts.DiagnosticsWithHotness,
+        CGOpts.DiagnosticsHotnessThreshold);
+    if (Error E = OptRecordFileOrErr.takeError()) {
+      handleAllErrors(
+        std::move(E),
+        [&](const RemarkSetupFileError &E) {
+        Diags.Report(diag::err_cannot_open_file)
+          << CGOpts.OptRecordFile << E.message();
+      },
+        [&](const RemarkSetupPatternError &E) {
+        Diags.Report(diag::err_drv_optimization_remark_pattern)
+          << E.message() << CGOpts.OptRecordPasses;
+      },
+        [&](const RemarkSetupFormatError &E) {
+        Diags.Report(diag::err_drv_optimization_remark_format)
+          << CGOpts.OptRecordFormat;
+      });
+      return false;
+    }
+    OptRecordFile = std::move(*OptRecordFileOrErr);
+
+    return true;
+  }
+
   class BackendConsumer : public ASTConsumer {
     using LinkModule = CodeGenAction::LinkModule;
 
     virtual void anchor();
     DiagnosticsEngine &Diags;
     BackendAction Action;
     const HeaderSearchOptions &HeaderSearchOpts;
     const CodeGenOptions &CodeGenOpts;
void HandleTranslationUnit(ASTContext &C) override {
       void *OldContext = Ctx.getInlineAsmDiagnosticContext();
       Ctx.setInlineAsmDiagnosticHandler(InlineAsmDiagHandler, this);
 
       std::unique_ptr<DiagnosticHandler> OldDiagnosticHandler =
           Ctx.getDiagnosticHandler();
       Ctx.setDiagnosticHandler(std::make_unique<ClangDiagnosticHandler>(
         CodeGenOpts, this));
 
-      Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
-          setupOptimizationRemarks(Ctx, CodeGenOpts.OptRecordFile,
-                                   CodeGenOpts.OptRecordPasses,
-                                   CodeGenOpts.OptRecordFormat,
-                                   CodeGenOpts.DiagnosticsWithHotness,
-                                   CodeGenOpts.DiagnosticsHotnessThreshold);
+      std::unique_ptr<llvm::ToolOutputFile> OptRecordFile;
+      bool isSet = setOptRecordFile(CodeGenOpts, Diags, Ctx, OptRecordFile);
 
-      if (Error E = OptRecordFileOrErr.takeError()) {
-        handleAllErrors(
-            std::move(E),
-            [&](const RemarkSetupFileError &E) {
-              Diags.Report(diag::err_cannot_open_file)
-                  << CodeGenOpts.OptRecordFile << E.message();
-            },
-            [&](const RemarkSetupPatternError &E) {
-              Diags.Report(diag::err_drv_optimization_remark_pattern)
-                  << E.message() << CodeGenOpts.OptRecordPasses;
-            },
-            [&](const RemarkSetupFormatError &E) {
-              Diags.Report(diag::err_drv_optimization_remark_format)
-                  << CodeGenOpts.OptRecordFormat;
-            });
+      // Error: the record file is erroneous.
+      if (!isSet)
         return;
-      }
-      std::unique_ptr<llvm::ToolOutputFile> OptRecordFile =
-          std::move(*OptRecordFileOrErr);
 
+      // There is a record file and it is not empty.
       if (OptRecordFile &&
           CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
         Ctx.setDiagnosticsHotnessRequested(true);
 
       // Link each LinkModule into our module.
       if (LinkInModules())
         return;
 
       EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef());
 
CodeGenAction::loadModule(MemoryBufferRef MBRef) {
   return {};
 }
 
 void CodeGenAction::ExecuteAction() {
   // If this is an IR file, we have to treat it specially.
   if (getCurrentFileKind().getLanguage() == Language::LLVM_IR) {
     BackendAction BA = static_cast<BackendAction>(Act);
     CompilerInstance &CI = getCompilerInstance();
+    auto &CodeGenOpts = CI.getCodeGenOpts();
     std::unique_ptr<raw_pwrite_stream> OS =
         GetOutputStream(CI, getCurrentFile(), BA);
     if (BA != Backend_EmitNothing && !OS)
       return;
 
     bool Invalid;
     SourceManager &SM = CI.getSourceManager();
     FileID FID = SM.getMainFileID();
     const llvm::MemoryBuffer *MainFile = SM.getBuffer(FID, &Invalid);
     if (Invalid)
       return;
 
     TheModule = loadModule(*MainFile);
     if (!TheModule)
       return;
 
     const TargetOptions &TargetOpts = CI.getTargetOpts();
     if (TheModule->getTargetTriple() != TargetOpts.Triple) {
       CI.getDiagnostics().Report(SourceLocation(),
                                  diag::warn_fe_override_module)
           << TargetOpts.Triple;
       TheModule->setTargetTriple(TargetOpts.Triple);
     }
 
-    EmbedBitcode(TheModule.get(), CI.getCodeGenOpts(),
+    EmbedBitcode(TheModule.get(), CodeGenOpts,
                  MainFile->getMemBufferRef());
 
     LLVMContext &Ctx = TheModule->getContext();
     Ctx.setInlineAsmDiagnosticHandler(BitcodeInlineAsmDiagHandler,
                                       &CI.getDiagnostics());
 
+    std::unique_ptr<llvm::ToolOutputFile> OptRecordFile;
+    bool isSet = setOptRecordFile(CodeGenOpts, CI.getDiagnostics(),
+                                  Ctx, OptRecordFile);
+
+    // Error: the record file is erroneous.
+    if (!isSet)
+      return;
+
     EmitBackendOutput(CI.getDiagnostics(), CI.getHeaderSearchOpts(),
-                      CI.getCodeGenOpts(), TargetOpts, CI.getLangOpts(),
+                      CodeGenOpts, TargetOpts, CI.getLangOpts(),
                       CI.getTarget().getDataLayout(), TheModule.get(), BA,
                       std::move(OS));
+
+    if (OptRecordFile)
+      OptRecordFile->keep();
+
     return;
   }
 
   // Otherwise follow the normal AST path.
   this->ASTFrontendAction::ExecuteAction();
 }
 
 //
 
 void EmitAssemblyAction::anchor() { }
 EmitAssemblyAction::EmitAssemblyAction(llvm::LLVMContext *_VMContext)
   : CodeGenAction(Backend_EmitAssembly, _VMContext) {}
 
 void EmitBCAction::anchor() { }
 EmitBCAction::EmitBCAction(llvm::LLVMContext *_VMContext)
   : CodeGenAction(Backend_EmitBC, _VMContext) {}
 
 void EmitLLVMAction::anchor() { }
 EmitLLVMAction::EmitLLVMAction(llvm::LLVMContext *_VMContext)
   : CodeGenAction(Backend_EmitLL, _VMContext) {}

clang/test/CodeGen/opt-record-1.c

+// RUN: %clang_cc1 %s -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
+// RUN: %clang_cc1 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
+// RUN: cat %t.opt |  FileCheck -check-prefix=CHECK -check-prefix=CHECK-PASSES %s
+
+
+void foo(int *a, int *b, int *c) {
+#pragma omp parallel for
+ for (int i = 0; i < 100; i++) {
+ a[i] = b[i] + c[i];
+ }
+}
+
+// CHECK: --- !Missed
+// CHECK: Pass:            inline
+// CHECK: Name:            NoDefinition
+// CHECK: Function:        foo
+// CHECK-PASSES: Pass: inline
+