MIR printer should lowercase sub-register names to be in sync with parser?

[markus]

Summary

The MIRParser seem to only expect lowercase sub-register names (see PerTargetMIParsingState::initNames2SubRegIndices()) but the MIRPrinter does not lowercase before printing.

Our target has some uppercase letters in sub-register names and as a result we cannot immediately import what was exported without first doing a manual lowercase of these letters.

[bjope]

I think it would be better to let the printer use the same convention as the specified in the definitions (as used in the code).
And then I think it is a bug in the parser, as it tries to be case insensitive by matching the parsed string against a lower case definition. It should also lower the parsed string if we want the parser to be case insensitive.

If we fix the parser instead, then we still would get debug printouts etc that matches the mixed case used in the C++ code. Or maybe I haven't understood the problem correctly.

[markus]

In D60311#1456007, @bjope wrote:

I think it would be better to let the printer use the same convention as the specified in the definitions (as used in the code).
And then I think it is a bug in the parser, as it tries to be case insensitive by matching the parsed string against a lower case definition. It should also lower the parsed string if we want the parser to be case insensitive.

If we fix the parser instead, then we still would get debug printouts etc that matches the mixed case used in the C++ code. Or maybe I haven't understood the problem correctly.

I agree that this should be fixed in the parser. All that is needed is to decide if it should be case sensitive or not.
To me case sensitivity makes most sense as the instruction and register names used could be rather complex for certain targets.

[arsenm]

Needs testcase.

I would assume these are case sensitive and appear exactly as defined in tablegen, not forced lowercase anywhere.

[bjope]

lib/CodeGen/MachineOperand.cpp

767

If the names should be both printed and parsed as lower case, then I think it would be better to update utils/TableGen/RegisterInfoEmitter.cpp to lower case all names in the SubRegIndexNameTable. That way we wouldn't have to mess around with lower casing names during runtime.

[bjope]

lib/CodeGen/MachineOperand.cpp

767

Besides, that would make all debug printouts consistent (all uses of getSubRegIndexName() would get the same casing for the name).

[bjope]

I'm suggesting to replace this with a solution where the subregister name is case sensitive (in MIR parser).
See: http://lists.llvm.org/pipermail/llvm-dev/2019-May/132081.html

[markus]

Superseded by https://reviews.llvm.org/D61499

lib/CodeGen/MachineOperand.cpp

if (TargetRegisterInfo::isVirtualRegister(Reg)) {
         MRI = &MF->getRegInfo();
       }
     }
 
     OS << printReg(Reg, TRI, 0, MRI);
     // Print the sub register.
     if (unsigned SubReg = getSubReg()) {
       if (TRI)
-        OS << '.' << TRI->getSubRegIndexName(SubReg);
+        OS << '.' << StringRef(TRI->getSubRegIndexName(SubReg)).lower();
       else
         OS << ".subreg" << SubReg;
     }
     // Print the register class / bank.
     if (TargetRegisterInfo::isVirtualRegister(Reg)) {
       if (const MachineFunction *MF = getMFIfAvailable(*this)) {
         const MachineRegisterInfo &MRI = MF->getRegInfo();
         if (IsStandalone || !PrintDef || MRI.def_empty(Reg)) {