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? > > I agree with the concern. I hoped that solving the problem by skipping the BB > like the (bit-rotted) debug code needs to would be a minor surgery. As things > look now, it may be better to remove the non-working sched_block debug counter > entirely and implement a good solution for the problem at hand. OK, if debug counter sched_block is useless and can be removed, then the proposed new skipping becomes the only actual need for the artificial resolve_forw_deps. > >> >> Would you please consider 2 ideas below. >> >> #1: >> After a brief look, I'm guessing this part is causing the problem: >> haifa-sched.cc <http://haifa-sched.cc/>:schedule_block(): >> === [1] >> /* Loop until all the insns in BB are scheduled. */ >> while ((*current_sched_info->schedule_more_p) ()) >> { >> perform_replacements_new_cycle (); >> do >> { >> start_clock_var = clock_var; >> >> clock_var++; >> >> advance_one_cycle (); > > As I understand, we have spurious calls to advance_one_cycle on basic block > boundaries, which don't model the hardware (the CPU doesn't see BB boundaries) > and cause divergence when passing through a debug-only BB which would not be > present at all without -g. > > Since EBBs and regions may not have jump targets in the middle, advancing > a cycle on BB boundaries does not seem well motivated. Can we remove it? > > Can we teach haifa-sched to emit RTX NOTEs with hashes of DFA states on BB > boundaries with -fcompare-debug is enabled? It should make the problem > readily detectable by -fcompare-debug even when scheduling did not diverge. Good idea! It would be easy to detect the inconsistent issue with such note. BR, Kewen
