mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#640170, @probinson wrote:
> In https://reviews.llvm.org/D28404#640090, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28404#640046, @probinson wrote:
> >
> > > "I don't care" doesn't seem like much of a principle.
> >
> >
>
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#640046, @probinson wrote:
> In https://reviews.llvm.org/D28404#639887, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28404#639874, @probinson wrote:
> >
> > > Over the weekend I had a thought: Why is -O0 so special here?
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#639874, @probinson wrote:
> Over the weekend I had a thought: Why is -O0 so special here? That is,
> after going to all this trouble to propagate -O0 to LTO, how does this
> generalize to propagating -O1 or any other specific -O
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: CMakeLists.txt:113
+check_linker_flag("-Wl,--section-ordering-file,${CLANG_ORDER_FILE}"
+ LINKER_ORDER_FILE_WORKS)
+ endif()
mehdi_amini added a comment.
In https://reviews.llvm.org/D24688#639158, @jyknight wrote:
> Since this requires everyone generating llvm IR to add this module attribute
> for optimal codegen,
Not sure yet how it'll end up, I'll revamp this.
See also:
mehdi_amini added a comment.
In https://reviews.llvm.org/D24688#638835, @pcc wrote:
> Didn't we figure out in the end that this could be a function attribute
> instead?
We did? You wrote in PR30403: "I had a brief look at what it would take to have
a per-function TLI, and I'm not convinced
mehdi_amini updated this revision to Diff 83510.
mehdi_amini added a comment.
Rebase
https://reviews.llvm.org/D24688
Files:
clang/lib/CodeGen/BackendUtil.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGen/nobuiltin.c
clang/test/CodeGenCUDA/flush-denormals.cu
mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:910-912
// OptimizeNone wins over OptimizeForSize and MinSize.
F->removeFnAttr(llvm::Attribute::OptimizeForSize);
F->removeFnAttr(llvm::Attribute::MinSize);
mehdi_amini updated this revision to Diff 83480.
mehdi_amini added a comment.
Forgot to update test/CodeGen/attr-naked.c
https://reviews.llvm.org/D28404
Files:
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Frontend/CodeGenOptions.def
clang/lib/CodeGen/CGOpenMPRuntime.cpp
mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:910-912
// OptimizeNone wins over OptimizeForSize and MinSize.
F->removeFnAttr(llvm::Attribute::OptimizeForSize);
F->removeFnAttr(llvm::Attribute::MinSize);
mehdi_amini updated this revision to Diff 83468.
mehdi_amini added a comment.
Address Paul's comment (remove useless block and add period to end comment)
https://reviews.llvm.org/D28404
Files:
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Frontend/CodeGenOptions.def
mehdi_amini marked 2 inline comments as done.
mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:900
+ ShouldAddOptNone &= !D->hasAttr();
+ if (ShouldAddOptNone) {
+B.addAttribute(llvm::Attribute::OptimizeNone);
probinson
mehdi_amini marked 6 inline comments as done.
mehdi_amini added inline comments.
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:760-762
Fn->removeFnAttr(llvm::Attribute::NoInline);
+ Fn->removeFnAttr(llvm::Attribute::OptimizeNone);
mehdi_amini updated this revision to Diff 83459.
mehdi_amini added a comment.
Address comments: reorganize the way ShouldAddOptNone is handled, hopefully
make it more easy to track.
Also after talking with Chandler on IRC, the source attribute "cold" does
not add the LLVM IR attribute "optsize"
This revision was automatically updated to reflect the committed changes.
Closed by commit rL291300: Add a cc1 option to force disabling lifetime-markers
emission from clang (authored by mehdi_amini).
Changed prior to commit:
https://reviews.llvm.org/D28385?vs=83321=83443#toc
Repository:
rL
mehdi_amini updated this revision to Diff 83441.
mehdi_amini added a comment.
Herald added subscribers: dschuff, jfb.
Fix one more conflicts with always_inline, and change some test check lines
https://reviews.llvm.org/D28404
Files:
clang/include/clang/Driver/CC1Options.td
mehdi_amini updated this revision to Diff 83433.
mehdi_amini added a comment.
Herald added a subscriber: jholewinski.
Fix minsize issue (conditional was reversed)
https://reviews.llvm.org/D28404
Files:
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Frontend/CodeGenOptions.def
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#638299, @probinson wrote:
> In https://reviews.llvm.org/D28404#638221, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28404#638217, @probinson wrote:
> >
> > > The patch as-is obviously has a massive testing cost, and it's
mehdi_amini added a comment.
> In order to simplify distributed build system integration, where actions
may be scheduled before the Thin Link which determines the list of
objects selected by the linker. The gold plugin currently will emit
0-sized index files for objects not selected by the link,
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#638217, @probinson wrote:
> Maybe instead, pass a flag to enable setting optnone on everything when the
> driver sees `-O0 -flto`?
I'm not fond of this: limiting discrepancy between LTO and non-LTO reduces the
LTO specific bugs
This revision was automatically updated to reflect the committed changes.
Closed by commit rL291276: Use CodegenOpts::less when creating a TargetMachine
for clang `-O1` (authored by mehdi_amini).
Changed prior to commit:
https://reviews.llvm.org/D28409?vs=83404=83408#toc
Repository:
rL LLVM
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D28362
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini updated this revision to Diff 83404.
mehdi_amini added a comment.
clang-format
https://reviews.llvm.org/D28409
Files:
clang/lib/CodeGen/BackendUtil.cpp
Index: clang/lib/CodeGen/BackendUtil.cpp
===
---
mehdi_amini created this revision.
mehdi_amini added reviewers: echristo, chandlerc.
mehdi_amini added a subscriber: cfe-commits.
Clang was initializing the TargetMachine with CodeGenOpt::Default
for https://reviews.llvm.org/owners/package/1/. This change is aligning it on
llc:
-O0: OptLevel =
mehdi_amini updated this revision to Diff 83391.
mehdi_amini added a comment.
Herald added a subscriber: wdng.
Remove spurious change
https://reviews.llvm.org/D28404
Files:
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Frontend/CodeGenOptions.def
mehdi_amini created this revision.
mehdi_amini added reviewers: chandlerc, rsmith.
mehdi_amini added subscribers: cfe-commits, dexonsmith.
Herald added a reviewer: tstellarAMD.
Herald added a subscriber: nhaehnle.
Amongst other, this will help LTO to correctly handle/honor files
compiled with O0,
mehdi_amini created this revision.
mehdi_amini added reviewers: chandlerc, rsmith.
mehdi_amini added a subscriber: cfe-commits.
This intended as a debugging/development flag only.
https://reviews.llvm.org/D28385
Files:
clang/include/clang/Driver/CC1Options.td
mehdi_amini added inline comments.
Comment at: lib/CodeGen/BackendUtil.cpp:65
+"Ignore an empty index file and perform non-ThinLTO compilation"),
+llvm::cl::init(false));
+
Is it common to do this in clang?
https://reviews.llvm.org/D28362
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D28153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini added a comment.
I mean the two checks being out-of-sync is weird, so I rather have them
reconciled.
Repository:
rL LLVM
https://reviews.llvm.org/D28153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini added inline comments.
Comment at: CMakeLists.txt:436
+
+ if(CLANG_ORDER_FILE AND NOT EXISTS ${CLANG_ORDER_FILE})
+string(FIND "${CLANG_ORDER_FILE}" "${CMAKE_CURRENT_BINARY_DIR}" PATH_START)
So why `if(CLANG_ORDER_FILE ` here?
Repository:
mehdi_amini added inline comments.
Comment at: utils/perf-training/CMakeLists.txt:61
+message(FATAL_ERROR "Output clang order file is not set")
+ endif()
+
I'm missing something: the code in the main CMakeLists seems to allows to have
an empty value for
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D28139
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
(I can't test because LLVM_USE_SANITIZE_COVERAGE seems broken on Darwin)
https://reviews.llvm.org/D28133
___
cfe-commits
mehdi_amini added inline comments.
Comment at: fuzz/CMakeLists.txt:3
+if( LLVM_USE_SANITIZE_COVERAGE )
+ set(LLVM_LINK_COMPONENTS support)
+
This is a dependency on libLLVMSupport right? Why is this needed?
https://reviews.llvm.org/D28133
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM! Exciting to see the new PM arriving in clang :)
Comment at: lib/CodeGen/BackendUtil.cpp:757
+ setCommandLineOpts();
+ cl::PrintOptionValues();
+
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D28047#629922, @chandlerc wrote:
> In https://reviews.llvm.org/D28047#629892, @mehdi_amini wrote:
>
> > LGTM, but please wait for @rnk to confirm :)
> >
mehdi_amini added a comment.
> That said, I agree with Reid that it is a bit surprising. After thinking
> about it a lot, the reason I personally find it surprising is actually that I
> find -O0 being the default surprising. But I'm OK with not changing that
> here, and maybe never changing
mehdi_amini added inline comments.
Comment at: lib/Frontend/CompilerInvocation.cpp:453
+// There is no effect at O0 when we won't inline anyways.
+if (Opts.OptimizationLevel > 1) {
+ const Option = InlineArg->getOption();
chandlerc wrote:
>
mehdi_amini added a comment.
LGTM, but please wait for @rnk to confirm :)
Also any opinion about a driver option -emit-raw-llvm as @joerg suggested?
https://reviews.llvm.org/D28047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini added inline comments.
Comment at: lib/Frontend/CompilerInvocation.cpp:453
+// There is no effect at O0 when we won't inline anyways.
+if (Opts.OptimizationLevel > 1) {
+ const Option = InlineArg->getOption();
mehdi_amini wrote:
> I'd
mehdi_amini added inline comments.
Comment at: lib/Frontend/CompilerInvocation.cpp:453
+// There is no effect at O0 when we won't inline anyways.
+if (Opts.OptimizationLevel > 1) {
+ const Option = InlineArg->getOption();
I'd switch the two if
mehdi_amini added a comment.
In https://reviews.llvm.org/D28053#629768, @rnk wrote:
> The big change here is that `clang -O0` now applies the noinline attribute
> everywhere. I can see why someone might expect things to work that way, but
> it seems surprising to me at first glance.
>
> Before
mehdi_amini added a comment.
> It would be awesome if attribute sets were a bit more FileCheck friendly, but
> oh well.
I've been wondering about that, what's the point of attribute sets in the
textual IR?
I understand the idea for saving space in the Bitcode, but the IR does not have
to use
mehdi_amini added a comment.
In https://reviews.llvm.org/D28047#629746, @rnk wrote:
> How about standardizing on -disable-llvm-passes instead of
> -disable-llvm-optzns? I never liked "optzns" and can't remember how to spell
> it. Also, this flag disables LLVM instrumentation passes (ASan) as
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM, but fix what @bruno spotted before committing.
https://reviews.llvm.org/D27832
___
cfe-commits mailing list
mehdi_amini added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:800
- void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); }
+ void makeLargest(bool Neg) {
+if (usesLayout(getSemantics())) {
jtony wrote:
> I know it is allowed to
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
Testing is in line with the existing testing.
It is not great, but at the same time is a development/debugging tool right?
Repository:
rL LLVM
https://reviews.llvm.org/D26964
mehdi_amini added a comment.
Is this covered by a driver test usually?
https://reviews.llvm.org/D27832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL289850: Fix printf specifier handling: invalid specifier
should not be marked as… (authored by mehdi_amini).
Changed prior to commit:
https://reviews.llvm.org/D27796?vs=81532=81621#toc
Repository:
mehdi_amini added a comment.
(Wait a little in case @Quuxplusone still has comments.)
Repository:
rL LLVM
https://reviews.llvm.org/D27163
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: lib/CodeGen/CodeGenFunction.cpp:1086
+ }
+ return true;
+}
Just a nit: reversing the if condition allows early exit.
mehdi_amini created this revision.
mehdi_amini added reviewers: rsmith, bruno, dexonsmith.
mehdi_amini added a subscriber: cfe-commits.
https://reviews.llvm.org/D27796
Files:
clang/include/clang/Analysis/Analyses/FormatString.h
clang/test/CodeGen/builtins.c
clang/test/Sema/format-strings.c
mehdi_amini added inline comments.
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:95
def err_drv_force_crash : Error<
- "failing because environment variable '%0' is set">;
+ "failing because %select{environment variable|option}0 '%1' is set">;
def
mehdi_amini abandoned this revision.
mehdi_amini added a comment.
Thanks for the information! Closing this.
https://reviews.llvm.org/D26376
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini added a comment.
In https://reviews.llvm.org/D26376#597614, @mclow.lists wrote:
> More info - The following code:
>
> #include
> int main () {}
>
>
> fails to compile on either gcc 6.2 (locally), gcc 7 head (online compiler) or
> MSVC (online compiler).
Interesting, that
mehdi_amini added a comment.
(You have a typo in the decription, you may want to fix it before commit)
https://reviews.llvm.org/D27332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D27332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rL LLVM
https://reviews.llvm.org/D27298
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mehdi_amini added inline comments.
Comment at: test/CodeGenCXX/return.cpp:21
+ // CHECK-NOSTRICT-NEXT: load
+ // CHECK-NOSTRICT-NEXT: ret i32
+ // CHECK-NOSTRICT-NEXT: }
rjmccall wrote:
> mehdi_amini wrote:
> > Quuxplusone wrote:
> > > Can you explain why a
mehdi_amini added inline comments.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+ // Don't need to mark Objective-C methods or blocks since the undefined
+ // behaviour optimization isn't used for them.
+}
Quuxplusone wrote:
> This seems like a trap
mehdi_amini added a comment.
In https://reviews.llvm.org/D27163#606744, @arphaman wrote:
> In https://reviews.llvm.org/D27163#606695, @mehdi_amini wrote:
>
> > What is the justification for a platform specific default change here?
>
>
> The flag itself is platform agnostic, however, the default
mehdi_amini added a comment.
What is the justification for a platform specific default change here?
Repository:
rL LLVM
https://reviews.llvm.org/D27163
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
301 - 363 of 363 matches
Mail list logo
