[PATCH] D115808: [DAGCombiner] Avoid combining adjacent stores at -O0 to improve debug experience

[Shivam Gupta via Phabricator via llvm-commits]

xgupta added a comment.

In D115808#3201928 <https://reviews.llvm.org/D115808#3201928>, @jrtc27 wrote:

> In D115808#3201773 <https://reviews.llvm.org/D115808#3201773>, @xgupta wrote:
>
>> I will commit as it seems not a big change. and all comments are addressed.
>
> This goes against the LLVM Code-Review Policy and Practices:
>
>> Code review can be an iterative process, which continues until the patch is ready to be committed. Specifically, once a patch is sent out for review, it needs an explicit approval before it is committed. Do not assume silent approval, or solicit objections to a patch with a deadline.
>
>
>
> - https://llvm.org/docs/CodeReview.html#code-review-workflow

Sorry, I didn't read it before, I will take care to not commit until patch is approved and all reviewers satisfy with the changes.

In D115808#3202913 <https://reviews.llvm.org/D115808#3202913>, @spatel wrote:

> I agree that it would be better to have a higher-level fix for the problem and made a similar comment here:
> D111596 <https://reviews.llvm.org/D111596>
>
> But it doesn't seem like there's enough motivation to overcome whatever the underlying bugs are, so we have a bunch of these 'optnone' checks around DAGCombiner now.

Sorry, I read it fast that I didn't understand the meaning, I think you agree with the change, for now, don't want to revert it, right?

> For reference, the original bug that mentioned disabling the whole combiner was:
> https://llvm.org/PR22346
>
> With more discussion here:
> https://lists.llvm.org/pipermail/llvm-dev/2016-May/099686.html

I did read it before but not thoroughly I think it is fine to disable to combining & canonicalization both if they affect debug info as mentioned in this mail by Robinson, Paul https://lists.llvm.org/pipermail/llvm-dev/2016-May/099772.html.

If someone accepts it will commit after a day. I doubt I can (may try) implement the registration mechanism that @qcolombet was talking about in that email thread.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115808/new/

https://reviews.llvm.org/D115808