Page MenuHomePhabricator

[MachineVerifier] Report allocatable classes for physical register copies
AbandonedPublic

Authored by lewis-revill on Jan 26 2022, 3:49 AM.

Details

Reviewers
asb
arsenm
Summary

The method getMinimalPhysRegClassLLT is used by the machine verifier on gMIR to find the register class for a physical register operand of a copy to/from a physical register. It then uses the register class to determine the size of the register to verify that the copy has matching sizes.

If this method can report unallocatable register classes as the best match for a physical register then we can run into the issue where, since unallocatable register classes are used as dummy classes where the type/size is unknown (IE for RISC-V), then the reported size doesn't match that of the register class that the register actually belongs to.

This may not be the only/neatest way of solving this issue but the issue is a blocker for RISC-V GlobalISel support.

Diff Detail

Event Timeline

lewis-revill created this revision.Jan 26 2022, 3:49 AM
lewis-revill requested review of this revision.Jan 26 2022, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2022, 3:50 AM
arsenm added inline comments.Jan 26 2022, 6:26 AM
llvm/lib/CodeGen/TargetRegisterInfo.cpp
229–242

This function was intended to follow along with the DAG version, which does not have the allocatable filter. It would probably be better to follow along with the pattern, and have the use have to explicitly filter with getAllocatableClass

lewis-revill added inline comments.Feb 1 2022, 8:25 AM
llvm/lib/CodeGen/TargetRegisterInfo.cpp
229–242

I can understand why it's best to match that. Problem is if this finds an unallocatable class first, even among multiple suitable allocatable classes then it will always report that. So the user can do nothing about it. I could try and rewrite it so that it reports the unallocatable class only if we don't find a suitable allocatable one. Another option is to add another property which matches this better than isAllocatable. Something like isDummyClass...

arsenm added inline comments.Feb 1 2022, 5:13 PM
llvm/lib/CodeGen/TargetRegisterInfo.cpp
229–242

I think it's important to keep the semantics of the two matching. What caller context are you having a problem? Code generally applies getAllocatableClass in conjunction with this

lewis-revill marked 2 inline comments as done.Feb 2 2022, 2:25 AM
lewis-revill added inline comments.
llvm/lib/CodeGen/TargetRegisterInfo.cpp
229–242

I looked into this again, and the problem was that I was always getting this unallocatable class reported with pointer LLTs. However the reason is not actually because the unallocatable class is overriding the allocatable class encountered later, but actually because isTypeLegalForClass always returns false for pointer LLTs (since no MVT types map to LLT pointer types). So I will create a new patch which addresses this instead. This would fix my issue so I think this patch isn't relevant - the allocatable class would always override the unallocatable class anyway if the type was legal.

lewis-revill abandoned this revision.Feb 2 2022, 2:39 AM
lewis-revill marked an inline comment as done.

Abandoned in favour of D118766