This commit should clarify semantics and issues related to reserved registers. Extend the machine verifier to check for the super register rule.
This is an early patch to get the discussion going. Some things are still missing:
- A bunch of targets violate the superreg rule (e.g. AArch64 reserving XZR but not tuples like %XZR_X0). AArch64, ARM, Hexagon, Mips and PowerPC currently fail the verifier. So far it looks like actual target bugs to me.
- I think we should add this rule: "If a reserved subregister has subregister at least one subregister (or register unit) must be reserved", yes?
Update comment, include the "at least 1 subregister must be reserved rule".
For the next version: I will add the check for the 1 subregister reserved rule; I'll move the checking from the machine verifier into MachineRegisterInfo::freezeReserved() (under #ifndef NDEBUG of course).
Thanks for clarifying that. That could lead to subtle bugs :).
I miss one thing in this patch: a helper function that given a set of reserved registers adds the super registers (transitively). Then hint in the comment that TargetRegisterInfo::getReservedRegs could use such helper.
Looks good to me.
Indeed, I am currently fixing the existing targets and added such a function for them anyway. BTW: It seems most of the problems are benign, a common pattern is the 64bit registers not getting reserved in 32bit mode. That should be fine as you probably won't see a 64bit operand in 32bit mode anyway. However fixing it properly is worth it so we can add the checking/assert.
These new rules break existing use in Mips :-(
Mips has 32bit floatregs F0-F31, each pair forms a double reg (F0+F1=D0, F2+F3=D1, ...). There is an option in the mips backend that aims at disabling the odd F registers, so 32bit float values only end up in F0, F2, F4, ... In this case it is actually expected that the register allocator still assigns the D0-D15 as usual even though the would indirectly use F1,F3,...
Is there another way to handle this in the mips target?
@MatzeB Yes, we could rework the floating point register set definitions and instruction definitions to use difference register classes depending the floating point ABI used. Unfortunately this requires six register classes in that case, three for the no odd single precision case and 3 more for the case where odd single precisions are available. Those register classes would eliminate the need to reserve the odd registers for no-oddsp.
That's a non-trivial change which will take some time as other plumbing may need to be fixed/changed.
The difficulty/delay arises in that MIPS has several different FPU configurations: 32bit FPUs supporting 64bit operations using a pair of single precision registers, and full 64bit FPUs. We have two floating pointing ABIs: nooddsp where the odd single precision registers are not to be used at all, and oddsp where all single precision registers can be used.
after working on this for a while I think the proposed rules here should rather be guidelines. They are the right thing in many situations, but not all. I reworked the commit in a way that backends can call assertAllSuperRegsMarked() at their own discretion.
- Make the rules less strict, marking superregs is only a recommendation now
- Add convenience functions so that it is easy in a target to mark all super regs and to verify whether all super regs are marked
- Fix a bug where AArch64 did not reserve tuples containing the zero register
If it's ignored by liveness tracking, then there is no meaning to "live" or "dead". Maybe you could rephrase it (and the previous bullet) as something like "is ignored from the liveness point of view".
This should be a standalone sentence, not a bullet in the list.
If you take a usual liveness definition of the value will potentially be read, then they are very much live.
Given that there are no arbitrary alias definitions in your target (I see no Aliases = in your tablegen files) it has the same effect as long as you only apply it to lowest level registers and not to a tuple.
We miss a return here or change the comment.
What about cases where we have mixed uses:
This API does not all to check that.
One suggestion though.
I'd change the API a bit:
The rationale is that in release build assert(checkAllSuperRegsMarked) can be optimized out, but assertAllSuperRegsMarked would still cause a call to an empty function.
Actually my first version returned a bool and had the assert() on the caller. I changed my mind because I wanted the display a message listing the register causing the problem and writing to stderr in a check function felt wrong. Would you think writing to stderr is fine anyway? Or should I just get rid of the message and have people figure out the problematic register in a debugger?