Functions marked 'optnone' should not be optimized. Per Chandler's request, we avoided adding new attributes that Pass Manager would have to be aware of, and had each pass individually ignore 'optnone' functions.
Details
Diff Detail
Event Timeline
Ping * 5. Hello? Phab review all refreshed and everything.
cc llvm-commits from phab, instead of pinging the original email thread, hopefully this will work.
Is there an email thread that decided on this direction? (so that I can catch up) It seems really odd (and difficult to maintain) to litter the codebase with checks in each pass.
In particular, what ever happened to just splitting all the notionally "optnone" functions into a separate module, and linking them back in at the end? That seems much more general. I don't see a response to http://article.gmane.org/gmane.comp.compilers.llvm.cvs/160085 or a response to Nick's original suggestion (which was restricted to using the commandline tools). It seems very simple to split the module like that.
-----Original Message-----
From: Sean Silva [mailto:silvas@purdue.edu]
Sent: Tuesday, January 14, 2014 6:57 PM
To: chandlerc@gmail.com; Robinson, Paul
Cc: llvm-commits@cs.uiuc.edu; silvas@purdue.edu
Subject: Re: [PATCH] Disable passes on optnone functionsIs there an email thread that decided on this direction? (so that Ican catch up) It seems really odd (and difficult to maintain) to litter
the codebase with checks in each pass.
There was this one, which has lots of other stuff on it and one comment
from Chandler not liking a less intrusive patch that added a flag and
caused the pass manager to make the decision instead of the individual
passes:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131111/194876.html
The implied model now is that every pass needs to be aware of the
attributes that would affect its operation, and Do The Right Thing(tm).
I'm not happy that it touches every transform pass either, but this is
specifically responding to Chandler's review comments. It doesn't
expand the PassManager<->Pass interface, at the cost of widening the
IR<->Pass interface.
In particular, what ever happened to just splitting all the notionally"optnone" functions into a separate module, and linking them back in at
the end? That seems much more general. I don't see a response to
http://article.gmane.org/gmane.comp.compilers.llvm.cvs/160085 or a
response to Nick's original suggestion (which was restricted to using
the commandline tools). It seems very simple to split the module like
that.
Except it totally fails for LTO. I mentioned this to Nick at the
LLVM conference, which he accepted as a valid objection; sorry that
never got documented on the list.
--paulr
Ping....
I tried to ping this from inside Phab but I'm not seeing the
notice show up on llvm-commits. Somehow Phab and I just don't
get along very well.
--paulr
-----Original Message-----
From: llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-
bounces@cs.uiuc.edu] On Behalf Of Paul Robinson
Sent: Tuesday, January 14, 2014 4:01 PM
To: llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] Disable passes on optnone functionsprobinson added you to the CC list for the revision "Disable passes on
optnone functions".Hi chandlerc,
Functions marked 'optnone' should not be optimized. Per Chandler's
request, we avoided adding new attributes that Pass Manager would have
to be aware of, and had each pass individually ignore 'optnone'
functions.http://llvm-reviews.chandlerc.com/D2369
Files:
lib/IR/LegacyPassManager.cpp lib/Analysis/LoopPass.cpp lib/Transforms/Scalar/JumpThreading.cpp lib/Transforms/Scalar/Reassociate.cpp lib/Transforms/Scalar/SimplifyCFGPass.cpp lib/Transforms/Scalar/DeadStoreElimination.cpp lib/Transforms/Scalar/SROA.cpp lib/Transforms/Scalar/EarlyCSE.cpp lib/Transforms/Scalar/ADCE.cpp lib/Transforms/Scalar/MemCpyOptimizer.cpp lib/Transforms/Scalar/TailRecursionElimination.cpp lib/Transforms/Scalar/ScalarReplAggregates.cpp lib/Transforms/Scalar/CorrelatedValuePropagation.cpp lib/Transforms/Scalar/SCCP.cpp lib/Transforms/Scalar/GVN.cpp lib/Transforms/Scalar/DCE.cpp lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Vectorize/SLPVectorizer.cpp lib/Transforms/Utils/LowerExpectIntrinsic.cpp lib/CodeGen/IfConversion.cpp lib/CodeGen/MachineLICM.cpp lib/CodeGen/PeepholeOptimizer.cpp lib/CodeGen/ProcessImplicitDefs.cpp lib/CodeGen/TailDuplication.cpp lib/CodeGen/EarlyIfConversion.cpp lib/CodeGen/DeadMachineInstructionElim.cpp lib/CodeGen/BranchFolding.cpp lib/CodeGen/OptimizePHIs.cpp lib/CodeGen/MachineSink.cpp lib/CodeGen/MachineCSE.cpp lib/CodeGen/MachineCopyPropagation.cpp test/Transforms/FunctionAttrs/optnone.ll
Please split the patch in two, one for changes to IR-level passes and pass manager (which should land first -- that should help disambiguate when there are pieces that both depend on), one for the codegen changes.
I'm unsure about the codegen changes here. You're adding a test at the beginning of "runOnMachineFunction" to each machine-level pass. The machine level requires a fixed set of passes to work in a certain way, you can't just disable some passes and expect it to work, the way you can with the IR-level passes.
The rationale for making the passes individually disable themselves on optnone is because some passes need to ignore optnone -- the always-inliner for example.
The test probably ought to go into test/Feature/optnone.ll ? Running -O1 and -O2, and opt and llc, it's not really testing the functionattrs pass.
I've reviewed everything except for lib/CodeGen/* and consider it good to commit. Sorry for the delays in review!
lib/Analysis/LoopPass.cpp | ||
---|---|---|
180 | This is a pass manager? | |
lib/IR/LegacyPassManager.cpp | ||
1293 | I thought you weren't going to add it to pass managers but to individual passes? This is a pass manager. |
-----Original Message-----
From: Nick Lewycky [mailto:nlewycky@google.com]
Sent: Wednesday, January 29, 2014 4:02 PM
To: chandlerc@gmail.com; Robinson, Paul
Cc: llvm-commits@cs.uiuc.edu; silvas@purdue.edu; nlewycky@google.com
Subject: Re: [PATCH] Disable passes on optnone functionsPlease split the patch in two, one for changes to IR-level passes andpass manager (which should land first -- that should help disambiguate
when there are pieces that both depend on), one for the codegen changes.
Okay. New patches coming, although it may take a little while, see
the comments below re. loop and basic-block testing.
I'm unsure about the codegen changes here. You're adding a test at thebeginning of "runOnMachineFunction" to each machine-level pass. The
machine level requires a fixed set of passes to work in a certain way,
you can't just disable some passes and expect it to work, the way you
can with the IR-level passes.
Let me get the revised IR-level patch done, then we can get back to the
machine-level part.
The rationale for making the passes individually disable themselves onoptnone is because some passes need to ignore optnone -- the always-
inliner for example.The test probably ought to go into test/Feature/optnone.ll ? Running -O1 and -O2, and opt and llc, it's not really testing the functionattrs
pass.
Okay, I misunderstood the intent of test/Transforms/FunctionAttrs.
I've reviewed everything except for lib/CodeGen/* and consider it goodto commit. Sorry for the delays in review!
Thanks for doing it! Now my plans for the next social can change
from "bring a laptop and two goons to make somebody review this" to
"lemme buy you a drink." :-)
Comment at: lib/IR/LegacyPassManager.cpp:1292
@@ -1291,1 +1291,3 @@return false;+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {
+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
I thought you weren't going to add it to pass managers but to individual
passes? This is a pass manager.Comment at: lib/Analysis/LoopPass.cpp:180
@@ -179,1 +179,3 @@
bool LPPassManager::runOnFunction(Function &F) {
+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
This is a pass manager?
BBPassManager and LPPassManager are themselves function passes.
I disabled the function passes that are the collection of basic-block
passes and the collection of loop passes, respectively, on the theory
that optnone is a function-level attribute. However, I can see given
events such as Chandler's recent conversion of a loop pass to a
function pass that disabling the passes individually could be safer.
I've worked out how to find the function attributes from a basic
block or loop. However it's really not feasible (AFAICT) to print out
a "skipping" diagnostic once per function, if the passes are disabled
at the loop or basic block level. That will make testing these things
distinctly more tedious; basically I'll have to take a functional test
for each optimization, slap 'optnone' on it, and verify that the
optimization did NOT happen. So it may take a little while before I
can turn in revised patches.
Thanks,
--paulr
Revised patch for IR-level passes attached. I got tired of
pasting the debug diagnostic so I factored the real work into
the pass base classes. For function and basic-block passes I
can make the debug output appear only once per function; didn't
see a good way to do that for loop passes. Not really critical.
Debug output does make it pretty easy to test. I also threw
in a check that optnone doesn't suppress anything at -O0.
Let's get this done and then I'll take another look at all the
machine-function passes.
Thanks,
--paulr
-----Original Message-----
From: llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-
bounces@cs.uiuc.edu] On Behalf Of Robinson, Paul
Sent: Thursday, January 30, 2014 2:30 PM
To: reviews+D2369+public+3a7363072c7f178d@llvm-reviews.chandlerc.com;
chandlerc@gmail.com
Cc: llvm-commits@cs.uiuc.edu
Subject: RE: [PATCH] Disable passes on optnone functions-----Original Message-----
From: Nick Lewycky [mailto:nlewycky@google.com]
Sent: Wednesday, January 29, 2014 4:02 PM
To: chandlerc@gmail.com; Robinson, Paul
Cc: llvm-commits@cs.uiuc.edu; silvas@purdue.edu; nlewycky@google.com
Subject: Re: [PATCH] Disable passes on optnone functionsPlease split the patch in two, one for changes to IR-level passesand
pass manager (which should land first -- that should help disambiguate
when there are pieces that both depend on), one for the codegenchanges.
Okay. New patches coming, although it may take a little while, see
the comments below re. loop and basic-block testing.I'm unsure about the codegen changes here. You're adding a test atthe
beginning of "runOnMachineFunction" to each machine-level pass. The
machine level requires a fixed set of passes to work in a certain way,
you can't just disable some passes and expect it to work, the way you
can with the IR-level passes.Let me get the revised IR-level patch done, then we can get back to the
machine-level part.The rationale for making the passes individually disable themselveson
optnone is because some passes need to ignore optnone -- the always-
inliner for example.The test probably ought to go into test/Feature/optnone.ll ? Running
O1 and -O2, and opt and llc, it's not really testing the functionattrs
pass.Okay, I misunderstood the intent of test/Transforms/FunctionAttrs.
I've reviewed everything except for lib/CodeGen/* and consider itgood
to commit. Sorry for the delays in review!
Thanks for doing it! Now my plans for the next social can change
from "bring a laptop and two goons to make somebody review this" to
"lemme buy you a drink." :-)Comment at: lib/IR/LegacyPassManager.cpp:1292
@@ -1291,1 +1291,3 @@return false;+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {
+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
I thought you weren't going to add it to pass managers but to
individual
passes? This is a pass manager.
Comment at: lib/Analysis/LoopPass.cpp:180
@@ -179,1 +179,3 @@
bool LPPassManager::runOnFunction(Function &F) {
+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
This is a pass manager?
BBPassManager and LPPassManager are themselves function passes.
I disabled the function passes that are the collection of basic-block
passes and the collection of loop passes, respectively, on the theory
that optnone is a function-level attribute. However, I can see given
events such as Chandler's recent conversion of a loop pass to a
function pass that disabling the passes individually could be safer.I've worked out how to find the function attributes from a basic
block or loop. However it's really not feasible (AFAICT) to print out
a "skipping" diagnostic once per function, if the passes are disabled
at the loop or basic block level. That will make testing these things
distinctly more tedious; basically I'll have to take a functional test
for each optimization, slap 'optnone' on it, and verify that the
optimization did NOT happen. So it may take a little while before I
can turn in revised patches.Thanks,
--paulr
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-----Original Message-----
From: Robinson, Paul
Sent: Monday, February 03, 2014 2:15 PM
To: Robinson, Paul; reviews+D2369+public+3a7363072c7f178d@llvm-
reviews.chandlerc.com; chandlerc@gmail.com
Cc: llvm-commits@cs.uiuc.edu
Subject: RE: [PATCH] Disable passes on optnone functionsRevised patch for IR-level passes attached.
Right. NOW they are attached.
I got tired of
pasting the debug diagnostic so I factored the real work into
the pass base classes. For function and basic-block passes I
can make the debug output appear only once per function; didn't
see a good way to do that for loop passes. Not really critical.
Debug output does make it pretty easy to test. I also threw
in a check that optnone doesn't suppress anything at -O0.Let's get this done and then I'll take another look at all the
machine-function passes.
Thanks,
--paulr-----Original Message-----
From: llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-
bounces@cs.uiuc.edu] On Behalf Of Robinson, Paul
Sent: Thursday, January 30, 2014 2:30 PM
To: reviews+D2369+public+3a7363072c7f178d@llvm-reviews.chandlerc.com;
chandlerc@gmail.com
Cc: llvm-commits@cs.uiuc.edu
Subject: RE: [PATCH] Disable passes on optnone functions-----Original Message-----
From: Nick Lewycky [mailto:nlewycky@google.com]
Sent: Wednesday, January 29, 2014 4:02 PM
To: chandlerc@gmail.com; Robinson, Paul
Cc: llvm-commits@cs.uiuc.edu; silvas@purdue.edu; nlewycky@google.com
Subject: Re: [PATCH] Disable passes on optnone functionsPlease split the patch in two, one for changes to IR-level passesand
pass manager (which should land first -- that should help
disambiguate
when there are pieces that both depend on), one for the codegen
changes.
Okay. New patches coming, although it may take a little while, see
the comments below re. loop and basic-block testing.I'm unsure about the codegen changes here. You're adding a test atthe
beginning of "runOnMachineFunction" to each machine-level pass. The
machine level requires a fixed set of passes to work in a certainway,
you can't just disable some passes and expect it to work, the way
you
can with the IR-level passes.
Let me get the revised IR-level patch done, then we can get back to
the
machine-level part.
The rationale for making the passes individually disablethemselves
on
optnone is because some passes need to ignore optnone -- the always-
inliner for example.The test probably ought to go into test/Feature/optnone.ll ?Running
O1 and -O2, and opt and llc, it's not really testing the
functionattrs
pass.
Okay, I misunderstood the intent of test/Transforms/FunctionAttrs.
I've reviewed everything except for lib/CodeGen/* and consider itgood
to commit. Sorry for the delays in review!
Thanks for doing it! Now my plans for the next social can change
from "bring a laptop and two goons to make somebody review this" to
"lemme buy you a drink." :-)Comment at: lib/IR/LegacyPassManager.cpp:1292
@@ -1291,1 +1291,3 @@return false;+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {
+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
I thought you weren't going to add it to pass managers but to
individual
passes? This is a pass manager.
Comment at: lib/Analysis/LoopPass.cpp:180
@@ -179,1 +179,3 @@
bool LPPassManager::runOnFunction(Function &F) {
+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {+ DEBUG(dbgs() << "Skipping pass '" << getPassName()
This is a pass manager?
BBPassManager and LPPassManager are themselves function passes.
I disabled the function passes that are the collection of basic-block
passes and the collection of loop passes, respectively, on the theory
that optnone is a function-level attribute. However, I can see given
events such as Chandler's recent conversion of a loop pass to a
function pass that disabling the passes individually could be safer.I've worked out how to find the function attributes from a basic
block or loop. However it's really not feasible (AFAICT) to print out
a "skipping" diagnostic once per function, if the passes are disabled
at the loop or basic block level. That will make testing these things
distinctly more tedious; basically I'll have to take a functional test
for each optimization, slap 'optnone' on it, and verify that the
optimization did NOT happen. So it may take a little while before I
can turn in revised patches.Thanks,
--paulr
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
- {F31726, layout=link}
Well, ultimately the revised content of this patch landed in r200892. The email record exists if you want to look at it, circa Feb 5 2014.
But by that time it was clear Phabricator hated me and I had stopped trying to do anything with it.
More recently I've been persuaded to try it again, and a few basic things do seem to work okay.
Phab's "help" is astoundingly un-helpful. If there's something you actually want me to _do_ here, please (a) point me to a Phab help page that describes it, or (b) explain in enough detail that I can document it on LLVM's Phab page.
Thanks,
--paulr
The little "comment" dropdown above the comment box (the one at the bottom
of the review, after all the diff changes) has an "abandon" option, which I
think is what we're doing for code reviews that end up not going anywhere.
(there's also a "close" option too, I'm not sure of the distinction)
Maybe we should be using whatever feature this refers to:
https://secure.phabricator.com/T4720 so that we can all just mark code
reviews as abandoned, rather than requesting the original author do so.
Okay, I see that. But in fact this review did "go somewhere" and a later version
of the patch was actually committed. So "abandon" is semantically wrong, even
if it would do something to cause it to not be on lists of open reviews.
(BTW I am completely sympathetic about getting it off lists of open reviews.
I'm just at a loss for how to navigate this, shall we say, help-challenged product.)
(there's also a "close" option too, I'm not sure of the distinction)
Not seeing that in the drop-down list of Actions, where is that?
A later edition of this patch was handled on the email thread, rather than directly in Phabricator, approved by Nick Lewycky and committed as r200892.
I'm trying to persuade Phabricator to close out the review to tidy things up.
This is a pass manager?