Page MenuHomePhabricator

Optimization record for bytecode input missing- PR44000
AcceptedPublic

Authored by zahiraam on Mon, Nov 25, 11:42 AM.

Details

Summary

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

Diff Detail

Event Timeline

zahiraam created this revision.Mon, Nov 25, 11:42 AM
Herald added a project: Restricted Project. · View Herald Transcript
erichkeane added a subscriber: erichkeane.
craig.topper added inline comments.
clang/lib/CodeGen/CodeGenAction.cpp
85

retruns -> returns

303

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

1126

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

1129

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 added inline comments.Mon, Nov 25, 5:38 PM
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 updated this revision to Diff 231069.Tue, Nov 26, 7:25 AM
zahiraam marked 7 inline comments as done.
zahiraam added inline comments.
clang/lib/CodeGen/CodeGenAction.cpp
1126

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 added inline comments.Tue, Nov 26, 9:02 AM
clang/lib/CodeGen/CodeGenAction.cpp
1129

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 updated this revision to Diff 231524.Fri, Nov 29, 5:59 AM

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

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.

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.

296

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
4

Why are the two -check-prefixes needed?

5

Extra blank line here.

19

Extra blank line here too.

zahiraam updated this revision to Diff 232137.Wed, Dec 4, 8:08 AM
zahiraam marked 7 inline comments as done.

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

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

Removed one.

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 updated this revision to Diff 232152.Wed, Dec 4, 8:44 AM
zahiraam marked an inline comment as done.

Changed it. Thanks.

thegameg accepted this revision.Wed, Dec 4, 9:14 AM

LGTM with one last nit, thanks!

clang/lib/CodeGen/CodeGenAction.cpp
1074–1075

Indentation looks a bit off here.

This revision is now accepted and ready to land.Wed, Dec 4, 9:14 AM
zahiraam updated this revision to Diff 232164.Wed, Dec 4, 9:44 AM
zahiraam marked an inline comment as done.

Fixed the indentation. Thanks.

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 updated this revision to Diff 232649.Fri, Dec 6, 2:21 PM

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

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

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 accepted this revision.Fri, Dec 6, 2:30 PM

LGTM

leonardchan accepted this revision.Fri, Dec 6, 3:23 PM

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

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

@leonardchan The fix was committed. Thanks.

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

Thanks.

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.

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.