Page MenuHomePhabricator

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

Authored by markus on Apr 5 2019, 1:28 AM.

Details

Reviewers
arphaman
MatzeB
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.

Diff Detail

Event Timeline

markus created this revision.Apr 5 2019, 1:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 1:29 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
bjope added a subscriber: bjope.Apr 5 2019, 2:37 AM

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 added a comment.Apr 5 2019, 3:00 AM

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 added a subscriber: arsenm.Apr 5 2019, 6:51 AM

Needs testcase.

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

bjope added inline comments.Apr 15 2019, 2:08 AM
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 added inline comments.Apr 15 2019, 2:10 AM
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 added a comment.Thu, May 2, 6:23 AM

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