MaggieYi added a comment. Thanks for reviewing the patch. I try to answer your questions, could you please let me know if this makes sense to you?
Regards, ================ Comment at: lib/Driver/Tools.cpp:4064 @@ -4046,1 +4063,3 @@ + // Add runtime flag for PS4 when PGO or Coverage are enabled. + if (getToolChain().getTriple().isPS4CPU()) ---------------- vsk wrote: > Profiling instrumentation seems orthogonal to PIC. Could you keep the calls > to `addPS4ProfileRTArgs` in their original locations? Because this patch is adding “--dependent-lib= libclang_rt.profile-x86_64.a” to the compiler command line (CC1), instead of adding –l to the linker command. I had to move the function “addPS4ProfileRTArgs” call from “ConstructPS4LinkJob” to the “ConstructJob” function. ================ Comment at: test/Driver/ps4-runtime-flags.c:8 @@ +7,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fprofile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs -fprofile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > This tests whether ArgList::hasFlag() works. It doesn't really stress your > patch. Please omit it. I believe these do test my patch. My patch has changed the code from “Args.hasArg(options::…)” to “Args.hasFlag(options:: …)”. This changes the behavior from simply requiring a switch (hasArg) to requiring the switches in the correct order (hasFlag). ================ Comment at: test/Driver/ps4-runtime-flags.c:10 @@ +9,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-arcs -fno-profile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > ^ Ditto. See response to line 8. ================ Comment at: test/Driver/ps4-runtime-flags.c:11 @@ +10,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fprofile-arcs -fno-profile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s ---------------- vsk wrote: > Looks identical to line 9. Unless I'm missing something, this should be > removed. Thanks. I will remove this line and upload a new patch. ================ Comment at: test/Driver/ps4-runtime-flags.c:14 @@ +13,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate=dir %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-generate -fprofile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > Omit? See response to line 8. ================ Comment at: test/Driver/ps4-runtime-flags.c:16 @@ +15,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate -fno-profile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > This one too. See response to line 8. ================ Comment at: test/Driver/ps4-runtime-flags.c:22 @@ +21,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate=somefile.profraw %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate -fprofile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s ---------------- vsk wrote: > And this. See response to line 8. ================ Comment at: test/Driver/ps4-runtime-flags.c:23 @@ +22,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate -fprofile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > Typo: `-fcoverage-mappinge` Thanks. I will upload a new patch to fix this. ================ Comment at: test/Driver/ps4-runtime-flags.c:24 @@ +23,3 @@ +// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s +// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s ---------------- vsk wrote: > The rest of these feel a bit redundant, but I haven't looked too closely. See response to line 8. http://reviews.llvm.org/D15222 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
