JIT: Eliminate RegTracker (#18179)

[cshung]

https://github.com/dotnet/coreclr/issues/18179
@BruceForstall
@CarolEidt

[cshung]

The experiment above show that this statement isn't always true.

Currently the RegTracker methods only call regSet->rsSetRegsModified(), however the register allocator should already be doing this for any registers it allocates, as well as any registers killed by an instruction.

Looking into the initial pull request (i.e. f9367b3), there are some assert hit. I have a hard time to reproduce them. I was able to reproduce one on them in the CodeGen::genFuncletProlog(BasicBlock* block) method, where we asserted the frame pointer is not used in line 9602 and then asserted again the frame pointer is used in line 9657. So I made a change (i.e. 7ea82a0). It does appear to solve some cases (so more CI tasks are passing), but not quite completely.

So I decide I would experiment with the big hammer, just change verifyRegUsed() to actually change the mask. In that case, all CI tasks passed. Apparently, this isn't the right thing to do, that was meant to be an experiment.

The key problem I have right now is that I am not sure what is wrong - why ain't the register already marked? I guess 7ea82a0 is the right fix, but there must be something else like that I can't find. What would be a good approach to find those missed marking?

[CarolEidt]

Thanks for the work thus far!

So I decide I would experiment with the big hammer, just change verifyRegUsed() to actually change the mask. In that case, all CI tasks passed. Apparently, this isn't the right thing to do, that was meant to be an experiment.

Actually, this is probably the right thing to do as a first change. It duplicates existing behavior, while removing the unnecessary RegTracker from the path.

So - it appears that my assertion that any register that's referenced should already be in the modified set wasn't correct. I would suggest that you simply make the change to call regSet->rsSetRegsModified() directly, and if you'd like, you can file an issue to track down the places where it hadn't already been called.

I'll have a look at the changes.

[CarolEidt]

I like these changes overall - just need a couple of function headers.
I think it would be good to get this merged (it will be great to eliminate this unnecessary class), and then work on determining where the registers aren't already been set as modified.

[CarolEidt]

These two methods need function headers. See https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md#9.4.1.

Under the "Notes" section I would add a note saying that this method is intended to be called during code generation, and should simply validate that the register (or registers) have already been added to the modified set. Then, add a TODO-Cleanup: comment saying that we need to identify the places where this has not been done.

[CarolEidt]

The key problem I have right now is that I am not sure what is wrong - why ain't the register already marked?

The FP case is one that could/should be done at the time that the frame type is determined (LinearScan::setFrameType()). It could be done by CodeGen::setFrameRequired(), but since we've moved most of the responsibility for setting the modified registers to the register allocator, I think it should probably be done directly by LinearScan::setFrameType().

What would be a good approach to find those missed marking?

Here's what I would suggest:

• First, get a jitdump for the method that's failing to JIT. I find that this is nearly always the best place to start.
• In the debugger, determine which register is not in the set:
  • If it is a register allocated by the register (i.e. it's a register on a GenTree node), then there's somewhere that the register allocator is failing to add a register to the modified set.
  • If it is a register used by the code generator in the process of generating code for a GenTree node, it's probably a register that should have been either in the kill set or an internal register for the node.
  • Otherwise, if it's a register used in the prolog or epilog, then that code in CodeGen needs to mark it.

[CarolEidt]

Awesome - thanks!