[PATCH] D83335: [ScheduleDAGRRList] Use std::*_heap() to keep candidate queue a heap.

Wed Jul 8 13:56:08 PDT 2020

fhahn added a comment.

In D83335#2139796 <https://reviews.llvm.org/D83335#2139796>, @efriedma wrote:

> > After looking at the code for the source order comperator, it looks like the score could change after units are scheduled as well in some edge cases.
>
> So AssertisHeap might fail?  I'm not really comfortable with that...

I am not sure if we want to leave them in either. The main reason to include them in the patch was to show how I tried to verify things behave sanely for a wide range of inputs. As mentioned earlier, not picking the best candidate here should not a big deal and it should happen very rarely (did not happen during bootstrap on X86 and various SPEC & MultiSource benchmarks). The selection should be deterministic across different compilers/C++ STLs because the comparator enforces a total order. Does that make sense?

> 
> 
>> We might even go further and limit the source order comperator to just the IR ordering and the queue IDs, because the real scheduling should happen in the machine scheduler.
> 
> Make this a separate patch, in case it has some unexpected side-effect, but sure, that makes sense.

yes that definitely needs to be separate. I'll need to do a more careful evaluation there, as changing the heuristic unfortunately impacts a bunch of test cases in small ways.

In D83335#2139810 <https://reviews.llvm.org/D83335#2139810>, @efriedma wrote:

> Also, maybe we could change the way we compute scheduling priority based on the size of the queue.  So keep the current scheduling for common cases, but switch to a simpler heuristic if the queue gets too large.

Are you referring to using the heap only once the queue grows larger than a threshold or deciding what scheduling heuristics to enable based on the size? I'll add back the original threshold back to the patch. I removed it to ensure the heap & assertions are applied as broadly as possible for verification.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83335