llvm dev - May 2016 - [RFC] Disabling DAG combines in /O0

Hi all,

The DAGCombiner pass actually runs even if the optimize level is set to None.
This can result in incorrect debug information or unexpected stepping/debugging
experience. Not to mention that having good stepping/debugging experience is the
major reason to compile at /O0.

I recently suggested a patch to disable one specific DAG combine at /O0 that
broke stepping on a particular case (http://reviews.llvm.org/D19268), but other
similar cases could appear. In the same way, another patch was submitted last
year for a similar reason (http://reviews.llvm.org/D7181).

So, since the DAGCombiner is in fact an optimization pass, could it be disabled
completely (or partially?) in /O0? And how should it be done?

For example, would this patch be too aggressive?

Index: DAGCombiner.cpp
==================================================================---
DAGCombiner.cpp        (revision 269301)
+++ DAGCombiner.cpp     (working copy)
@@ -1251,6 +1251,10 @@
//===----------------------------------------------------------------------===//

 void DAGCombiner::Run(CombineLevel AtLevel) {
+
+  if (OptLevel == CodeGenOpt::None)
+    return;
+
   // set the instance variables, so that the various visit routines may use it.
   Level = AtLevel;
   LegalOperations = Level >= AfterLegalizeVectorOps;

It would most likely break some CodeGen tests since it would have an impact on
the code produced at /O0. In fact, I tried to run the CodeGen lit tests with
this patch and got 25 new test failures on different targets. These tests would
probably need to be updated.

Thanks,
Marianne

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160513/5a5ad2fc/attachment.html>

Hi Marianne,

This has been tried before. The problem is that although, ideally,
DAGCombine should only be used for optimization, in practice there exist
some combines that are required for correctness. So making this work
properly may require a fairly large clean-up job and a lot of testing.
I suggest you look at PR22346, http://reviews.llvm.org/D8614 and
http://reviews.llvm.org/D9992 for context.

Michael

On Fri, May 13, 2016 at 12:21 PM, Marianne Mailhot-Sarrasin via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi all,
>
>
>
> The DAGCombiner pass actually runs even if the optimize level is set to
> None. This can result in incorrect debug information or unexpected
> stepping/debugging experience. Not to mention that having good
> stepping/debugging experience is the major reason to compile at /O0.
>
>
>
> I recently suggested a patch to disable one specific DAG combine at /O0
> that broke stepping on a particular case (http://reviews.llvm.org/D19268),
> but other similar cases could appear. In the same way, another patch was
> submitted last year for a similar reason (http://reviews.llvm.org/D7181).
>
>
>
> So, since the DAGCombiner is in fact an optimization pass, could it be
> disabled completely (or partially?) in /O0? And how should it be done?
>
>
>
> For example, would this patch be too aggressive?
>
>
> Index: DAGCombiner.cpp
>
> ==================================================================>
> --- DAGCombiner.cpp        (revision 269301)
>
> +++ DAGCombiner.cpp     (working copy)
>
> @@ -1251,6 +1251,10 @@
>
>
>
//===----------------------------------------------------------------------===//
>
>
>
>  void DAGCombiner::Run(CombineLevel AtLevel) {
>
> +
>
> +  if (OptLevel == CodeGenOpt::None)
>
> +    return;
>
> +
>
>    // set the instance variables, so that the various visit routines may
> use it.
>
>    Level = AtLevel;
>
>    LegalOperations = Level >= AfterLegalizeVectorOps;
>
>
>
> It would most likely break some CodeGen tests since it would have an
> impact on the code produced at /O0. In fact, I tried to run the CodeGen lit
> tests with this patch and got 25 new test failures on different targets.
> These tests would probably need to be updated.
>
>
>
> Thanks,
>
> Marianne
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160513/13027cf7/attachment.html>

(Remembering to re-add llvm-dev again...)
Yes, this would be too aggressive.  DAG combiner does more than just
optimization (which I discovered as part of D7181, see PR22346).
Internally we've thrown around other ideas, for example inside the worklist
loop, skip loads and stores at O0, but haven't done any actual experiments. 
There are probably other bits of the combiner that could be turned off at O0,
but I think they'd have to be considered individually.  If you want to run
with this one, go right ahead.
--paulr

Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
==================================================================---
lib/CodeGen/SelectionDAG/DAGCombiner.cpp     (revision 269398)
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp     (working copy)
@@ -1300,6 +1300,10 @@
         continue;
     }

+    if (OptLevel == CodeGenOpt::None &&
+        (N->getOpcode() == ISD::LOAD || N->getOpcode() == ISD::STORE))
+      continue;
+
     DEBUG(dbgs() << "\nCombining: "; N->dump(&DAG));

     // Add any operands of the new node which have not yet been combined to the



From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Marianne
Mailhot-Sarrasin via llvm-dev
Sent: Friday, May 13, 2016 12:21 PM
To: llvm-dev at lists.llvm.org
Subject: [llvm-dev] [RFC] Disabling DAG combines in /O0

Hi all,

The DAGCombiner pass actually runs even if the optimize level is set to None.
This can result in incorrect debug information or unexpected stepping/debugging
experience. Not to mention that having good stepping/debugging experience is the
major reason to compile at /O0.

I recently suggested a patch to disable one specific DAG combine at /O0 that
broke stepping on a particular case (http://reviews.llvm.org/D19268), but other
similar cases could appear. In the same way, another patch was submitted last
year for a similar reason (http://reviews.llvm.org/D7181).

So, since the DAGCombiner is in fact an optimization pass, could it be disabled
completely (or partially?) in /O0? And how should it be done?

For example, would this patch be too aggressive?

Index: DAGCombiner.cpp
==================================================================---
DAGCombiner.cpp        (revision 269301)
+++ DAGCombiner.cpp     (working copy)
@@ -1251,6 +1251,10 @@
//===----------------------------------------------------------------------===//

 void DAGCombiner::Run(CombineLevel AtLevel) {
+
+  if (OptLevel == CodeGenOpt::None)
+    return;
+
   // set the instance variables, so that the various visit routines may use it.
   Level = AtLevel;
   LegalOperations = Level >= AfterLegalizeVectorOps;

It would most likely break some CodeGen tests since it would have an impact on
the code produced at /O0. In fact, I tried to run the CodeGen lit tests with
this patch and got 25 new test failures on different targets. These tests would
probably need to be updated.

Thanks,
Marianne

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160513/cb4b95ac/attachment.html>

Hi,

DAG combiner does indeed optimizations but also canonicalization.
The latter, we probably need to keep, the former, we should be able to disable.

When I asked Marianne to do a RFC about this, I was hoping we could get new
ideas on how to tackle this problem.

I am fine with the approach of disabling the optimizations one by one when we
know it is indeed an optimization. However, I am not a fan of having "if
(OptLevel == <...>)" scattered all around the place.
Instead, I would like to have a sort of list of optimizations that differ
between the opt level == none and the other opt level, then we run just that
list.

In other words, I was thinking we could have some kind of registration mechanism
for each optimization and this is a matter of classifying them.

Anyhow, what are other people thoughts?

Note: I understand this is a big refactoring and maybe not one that we want to
do now, but I believe we could reuse some of the ideas we are discussing here
when we eventually do combines in GobalISel.

Cheers,
-Quentin
> On May 13, 2016, at 12:51 PM, Robinson, Paul via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> (Remembering to re-add llvm-dev again…)
> Yes, this would be too aggressive.  DAG combiner does more than just
optimization (which I discovered as part of D7181, see PR22346).
> Internally we've thrown around other ideas, for example inside the
worklist loop, skip loads and stores at O0, but haven't done any actual
experiments.  There are probably other bits of the combiner that could be turned
off at O0, but I think they'd have to be considered individually.  If you
want to run with this one, go right ahead.
> --paulr
>  
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ==================================================================> ---
lib/CodeGen/SelectionDAG/DAGCombiner.cpp     (revision 269398)
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp     (working copy)
> @@ -1300,6 +1300,10 @@
>          continue;
>      }
>  
> +    if (OptLevel == CodeGenOpt::None &&
> +        (N->getOpcode() == ISD::LOAD || N->getOpcode() ==
ISD::STORE))
> +      continue;
> +
>      DEBUG(dbgs() << "\nCombining: ";
N->dump(&DAG));
>  
>      // Add any operands of the new node which have not yet been combined
to the
>  
>  
>   <>
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of
Marianne Mailhot-Sarrasin via llvm-dev
> Sent: Friday, May 13, 2016 12:21 PM
> To: llvm-dev at lists.llvm.org
> Subject: [llvm-dev] [RFC] Disabling DAG combines in /O0
>  
> Hi all,
>  
> The DAGCombiner pass actually runs even if the optimize level is set to
None. This can result in incorrect debug information or unexpected
stepping/debugging experience. Not to mention that having good
stepping/debugging experience is the major reason to compile at /O0.
>  
> I recently suggested a patch to disable one specific DAG combine at /O0
that broke stepping on a particular case (http://reviews.llvm.org/D19268
<http://reviews.llvm.org/D19268>), but other similar cases could appear.
In the same way, another patch was submitted last year for a similar reason
(http://reviews.llvm.org/D7181 <http://reviews.llvm.org/D7181>).
>  
> So, since the DAGCombiner is in fact an optimization pass, could it be
disabled completely (or partially?) in /O0? And how should it be done?
>  
> For example, would this patch be too aggressive?
> 
> Index: DAGCombiner.cpp
> ==================================================================> ---
DAGCombiner.cpp        (revision 269301)
> +++ DAGCombiner.cpp     (working copy)
> @@ -1251,6 +1251,10 @@
>
//===----------------------------------------------------------------------===//
>  
>  void DAGCombiner::Run(CombineLevel AtLevel) {
> +
> +  if (OptLevel == CodeGenOpt::None)
> +    return;
> +
>    // set the instance variables, so that the various visit routines may
use it.
>    Level = AtLevel;
>    LegalOperations = Level >= AfterLegalizeVectorOps;
>  
> It would most likely break some CodeGen tests since it would have an impact
on the code produced at /O0. In fact, I tried to run the CodeGen lit tests with
this patch and got 25 new test failures on different targets. These tests would
probably need to be updated.
>  
> Thanks,
> Marianne
>  
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160516/281fe460/attachment-0001.html>