This revision was automatically updated to reflect the committed changes.
Closed by commit rL304127: IRGen: Add optnone attribute on function during O0
(authored by mehdi_amini).
Changed prior to commit:
https://reviews.llvm.org/D28404?vs=83480=100580#toc
Repository:
rL LLVM
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
Actually, looking through the comments, it appears that everyone (eventually)
agreed with the approach in the patch. I agree too. LGTM.
Mehdi, are you able to rebase and commit, or
MatzeB added a comment.
FWIW, I think this makes sense.
Moving O0 and optnone get closer seems sensible. Even though -O3 with an
optnone function indeed gives you different results today.
We are basically maintaining two things for the same "do not optimize" goal.
This obviously won't make O0
probinson added a comment.
In https://reviews.llvm.org/D28404#675687, @chandlerc wrote:
> In https://reviews.llvm.org/D28404#675616, @mehdi_amini wrote:
>
> > We're still waiting for @rsmith to comment whether it'd be better to `have
> > a LangOpts flag that basically means "pragma clang
chandlerc added a comment.
Just to be explicit, I agree with Hal's summary. This seems like the right
engineering tradeoff and I don't find anything particularly unsatisfying about
it.
In https://reviews.llvm.org/D28404#675616, @mehdi_amini wrote:
> Also note that @chandlerc in r290398 made
mehdi_amini added a comment.
Also note that @chandlerc in r290398 made clang adding "noinline" on every
function at O0 by default, which seems very similar to what I'm doing here.
We're still waiting for @rsmith to comment whether it'd be better to `have a
LangOpts flag that basically means
hfinkel added a comment.
In https://reviews.llvm.org/D28404#673260, @mehdi_amini wrote:
> Ping :)
To clarify my understanding of this thread, it seems like there are three ways
forward here:
1. To have -O0 add optnone to the generated functions (enabling some degree of
lack of optimization
mehdi_amini added a comment.
Ping :)
https://reviews.llvm.org/D28404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
probinson added a comment.
@rsmith could you say whether it seems reasonable to have a LangOpts flag that
basically means "`pragma clang optimize off` is always in effect." I think it
would make the other optnone-related logic simpler. It would not be the only
sort-of-codegen-related flag in
probinson added a comment.
I guess I'm getting irritated because people are trying to tell me what optnone
means. I know what it means; I spent probably a whole year pushing to get it
adopted.
Optnone means: When you are running optimizations, try not to optimize this
part, if you can.
chandlerc added a comment.
In https://reviews.llvm.org/D28404#641696, @mehdi_amini wrote:
> In https://reviews.llvm.org/D28404#641632, @probinson wrote:
>
> > In https://reviews.llvm.org/D28404#641606, @mehdi_amini wrote:
> >
> > > If we want to support `-O0 -flto` and `optnone` it the way to
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#641632, @probinson wrote:
> In https://reviews.llvm.org/D28404#641606, @mehdi_amini wrote:
>
> > If we want to support `-O0 -flto` and `optnone` it the way to convey this
> > to the optimizer, I don't see the alternative.
>
>
>
probinson added a comment.
In https://reviews.llvm.org/D28404#641606, @mehdi_amini wrote:
> If we want to support `-O0 -flto` and `optnone` it the way to convey this to
> the optimizer, I don't see the alternative.
optsize != -Os (according to Chandler)
minsize != -Oz (according to Chandler)
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#641597, @probinson wrote:
> In https://reviews.llvm.org/D28404#641557, @mehdi_amini wrote:
>
> > As I stand right now, there hasn't been any correction.
> > I still consider the fact that `optnone` wouldn't produce the "same"
> >
probinson added a comment.
In https://reviews.llvm.org/D28404#641557, @mehdi_amini wrote:
> As I stand right now, there hasn't been any correction.
> I still consider the fact that `optnone` wouldn't produce the "same" result
> (modulo corner cases around `merging global variables` for
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#641538, @probinson wrote:
> > - optnone isn't *really* no optimizations: clearly this is true, but then
> > neither is -O0. We run the always inliner, a couple of other passes, and we
> > run several parts of the code generators
probinson added a comment.
In https://reviews.llvm.org/D28404#641078, @chandlerc wrote:
> For me, the arguments you're raising against -O0 and -flto don't hold up on
> closer inspection:
>
> - O0 != optnone: correct. But this is only visible in LTO. And in LTO, Os !=
> optsize, and Oz !=
chandlerc added a comment.
In https://reviews.llvm.org/D28404#640862, @probinson wrote:
> In https://reviews.llvm.org/D28404#640682, @mehdi_amini wrote:
>
> > > I'm now thinking along the lines of a `-foptimize-off` flag (bikesheds
> > > welcome) which would set the default for the pragma to
s.llvm.org
> Cc: nhaeh...@gmail.com; wei.di...@amd.com; jholewin...@nvidia.com; Richard
> Smith; cfe-commits; Peter Collingbourne
> Subject: Re: [PATCH] D28404: IRGen: Add optnone attribute on function
> during O0
>
> This seems like a massive rehash of a discussion Peter Collingbou
probinson added a comment.
In https://reviews.llvm.org/D28404#640682, @mehdi_amini wrote:
> > I'm now thinking along the lines of a `-foptimize-off` flag (bikesheds
> > welcome) which would set the default for the pragma to 'off'. How is that
> > different than what you wanted for `-O0`? It
This seems like a massive rehash of a discussion Peter Collingbourne and I had
about passing -O0 to the linker for -flto=full. I had previously thought of
LTO as "link time optimization", but in practice it's useful for (and required
for correctness of some) non-optimization IR passes.
In
mehdi_amini added a comment.
> I'm now thinking along the lines of a `-foptimize-off` flag (bikesheds
> welcome) which would set the default for the pragma to 'off'. How is that
> different than what you wanted for `-O0`? It is defined in terms of an
> existing pragma, which is WAY easier
probinson added a comment.
In https://reviews.llvm.org/D28404#640588, @mehdi_amini wrote:
> Actually, as mentioned before, I could be fine with making `O0` incompatible
> with LTO, however security features like CFI (or other sort of whole-program
> analyses/instrumentations) requires LTO.
mehdi_amini added a comment.
Actually, as mentioned before, I could be fine with making `O0` incompatible
with LTO, however security features like CFI (or other sort of whole-program
analyses/instrumentations) requires LTO.
https://reviews.llvm.org/D28404
probinson added a comment.
In https://reviews.llvm.org/D28404#640362, @probinson wrote:
> In https://reviews.llvm.org/D28404#640314, @mehdi_amini wrote:
>
> > I don't follow: IMO if I generate a module with optnone and pipe it to `opt
> > -O3` I expect no function IR to be touched. If it is not
probinson added a comment.
Basically, I don't see why having clang always emit a real .o at -O0 would be a
problem.
I haven't gotten through the other-CFI documentation yet though.
https://reviews.llvm.org/D28404
___
cfe-commits mailing list
probinson added a comment.
In https://reviews.llvm.org/D28404#640314, @mehdi_amini wrote:
> In https://reviews.llvm.org/D28404#640284, @probinson wrote:
>
> > In https://reviews.llvm.org/D28404#640178, @mehdi_amini wrote:
> >
> > > In https://reviews.llvm.org/D28404#640170, @probinson wrote:
> >
probinson added a comment.
In https://reviews.llvm.org/D28404#640314, @mehdi_amini wrote:
> In https://reviews.llvm.org/D28404#640284, @probinson wrote:
>
> > Upfront, it seemed peculiar to handle only one optimization level. After
> > more thought, the whole idea of mixing -O0 and LTO seems
probinson added a comment.
In https://reviews.llvm.org/D28404#640314, @mehdi_amini wrote:
> You just wrote above that " mixing -O0 and LTO " is wrong, *if* I were to
> agree with you at some point, then I'd make it a hard error.
Yes, I was not clear that I meant that `-O0 -flto` on the same
probinson added a comment.
In https://reviews.llvm.org/D28404#640314, @mehdi_amini wrote:
> I don't follow: IMO if I generate a module with optnone and pipe it to `opt
> -O3` I expect no function IR to be touched. If it is not the case it is a bug.
Your opinion and expectation are not
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#640297, @probinson wrote:
> Sorry, you lost me. CFI is part of DWARF and we do DWARF perfectly well
> without LTO (and at O0).
This CFI: http://clang.llvm.org/docs/ControlFlowIntegrity.html
https://reviews.llvm.org/D28404
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#640284, @probinson wrote:
> In https://reviews.llvm.org/D28404#640178, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28404#640170, @probinson wrote:
> >
> > > In https://reviews.llvm.org/D28404#640090, @mehdi_amini wrote:
>
probinson added a comment.
In https://reviews.llvm.org/D28404#640182, @mehdi_amini wrote:
> In https://reviews.llvm.org/D28404#640178, @mehdi_amini wrote:
>
> > Also, that's not practicable: what if I have an LTO static library for
> > which I don't have the source, now if I build my own file
probinson added a comment.
In https://reviews.llvm.org/D28404#640178, @mehdi_amini wrote:
> 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:
> >
mehdi_amini added a comment.
In https://reviews.llvm.org/D28404#640178, @mehdi_amini wrote:
> Also, that's not practicable: what if I have an LTO static library for which
> I don't have the source, now if I build my own file with -O0 -flto I can't
> link anymore.
Also: LTO is required for
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.
> >
> >
>
probinson added a comment.
In https://reviews.llvm.org/D28404#640170, @probinson wrote:
> In my experience, modifying source
Note that the source modification consists of adding `#pragma clang optimize
off` to the top of the file. It is not a complicated thing.
probinson added a comment.
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.
>
>
> Long version is: "There is no use-case, no users, so I don't have much
>
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?
probinson added a comment.
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? That is,
> > after going to all this trouble to propagate -O0 to LTO,
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
probinson added a comment.
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 option? (Maybe this question would be
better dealt with on the dev
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);
chandlerc 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);
probinson 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
probinson added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:896
+ !CodeGenOpts.DisableO0ImplyOptNone && CodeGenOpts.OptimizationLevel == 0;
+ // We can't add optnone in the following cases, it won't pass the verifier
+ ShouldAddOptNone &=
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"
probinson added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:962
+ ShouldAddOptNone &= !D->hasAttr();
+ ShouldAddOptNone &= !D->hasAttr();
+ ShouldAddOptNone &= !F->hasFnAttribute(llvm::Attribute::AlwaysInline);
chandlerc wrote:
> why is
chandlerc added inline comments.
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:760-762
Fn->removeFnAttr(llvm::Attribute::NoInline);
+ Fn->removeFnAttr(llvm::Attribute::OptimizeNone);
Fn->addFnAttr(llvm::Attribute::AlwaysInline);
At point where we are
probinson added a comment.
In https://reviews.llvm.org/D28404#638350, @mehdi_amini wrote:
> 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:
> >
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
probinson added a comment.
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 easy to
> > imagine people being tripped up by this in the future.
>
>
>
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
probinson added a comment.
Maybe instead, pass a flag to enable setting optnone on everything when the
driver sees `-O0 -flto`? The patch as-is obviously has a massive testing cost,
and it's easy to imagine people being tripped up by this in the future.
https://reviews.llvm.org/D28404
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,
63 matches
Mail list logo
