[PATCH] D118233: [MachineVerifier] Report allocatable classes for physical register copies

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill marked 2 inline comments as done.
lewis-revill added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetRegisterInfo.cpp:229-242
 TargetRegisterInfo::getMinimalPhysRegClassLLT(MCRegister reg, LLT Ty) const {
   assert(Register::isPhysicalRegister(reg) &&
          "reg must be a physical register");
 
   // Pick the most sub register class of the right type that contains
   // this physreg.
   const TargetRegisterClass *BestRC = nullptr;
----------------
arsenm wrote:
> lewis-revill wrote:
> > arsenm wrote:
> > > 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
> > 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`...
> 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118233/new/

https://reviews.llvm.org/D118233