On 11/9/23 18:57, Kewen.Lin wrote:
Hi Maxim and Alexander,
Thanks a lot for the review comments!
on 2023/11/10 01:40, Alexander Monakov wrote:
On Thu, 9 Nov 2023, Maxim Kuvyrkov wrote:
Hi Kewen,
Below are my comments. I don't want to override Alexander's review, and if
the patch looks good to him, it's fine to ignore my concerns.
My main concern is that this adds a new entity -- forceful skipping of
DEBUG_INSN-only basic blocks -- to the scheduler for a somewhat minor change
in behavior. Unlike NOTEs and LABELs, DEBUG_INSNs are INSNS, and there is
already quite a bit of logic in the scheduler to skip them _as part of normal
operation_.
Yeah, I noticed that the scheduler takes care of DEBUG_INSNs as normal
operations.
When I started to work on this issue, initially I wanted to try something
similar
to your idea #2, but when checking the APIs, I realized why not just skip the
basic
block with NOTEs and LABELs, DEBUG_INSNs as well. IMHO there is no value to
try to
schedule this kind of BB (to be scheduled range), skipping it can save some
resource
allocation (like block dependencies) and make it more efficient (not enter
function
schedule_block etc.), from this perspective it seems an enhancement. Does it
sound
reasonable to you?
It sounds reasonable, but only if doing so doesn't add significant
implementation complexity. ie, the gains from doing less work here are
likely to be very marginal, so I'm more interested in clean, easy to
maintain code.
Jeff
