RDF enbale fail in computePhiInfo

LLVM Project
1

Hi Krzysztof,
I have working to anble the RDF in our arch. and i meet some issue in rdf::Liveness::computePhiInfo().
It often fail in calling the PhysicalRegisterInfo::mapTo() to get the sub-register.
I debug into the function and found it failed end reported the llvm_unreachable(“Invalid arguments: unrelated registers?”) . because the function PhysicalRegisterInfo::mapTo() get the same vector element register sunch as ‘‘v4r0 and v4r1’’. they are not subsequnce of each other.
i have wondered do it need to support this situation and return a register like:
v4r0:[0 1 1 1]， r1 r2 r3 is active in its lanemask.

the patch below to work around this issue:

--- a/llvm/lib/CodeGen/RDFRegisters.cpp
+++ b/llvm/lib/CodeGen/RDFRegisters.cpp
@@ -239,7 +239,12 @@ RegisterRef PhysicalRegisterInfo::mapTo(RegisterRef RR, unsigned R) const {
     LaneBitmask M = TRI.reverseComposeSubRegIndexLaneMask(Idx, RR.Mask);
     return RegisterRef(R, M & RCM);
   }
-  llvm_unreachable("Invalid arguments: unrelated registers?");
+
+  RegisterRef RetR =
+      RegisterAggr(*this).insert(RR).intersectWith(RegisterRef(R));
+  if (!RetR)
+    llvm_unreachable("Invalid arguments: unrelated registers?");
+  return RetR;
 }

i think we just need to return the intersection of the sub-register.

Thanks in advance.

CC: @kparzysz-quic

2

Yes, this should be fine. The mapTo function should eventually be removed, since the essential functions in RDF now work in terms of register units. In the past two RegisterRefs would compare equal if the register ids and the lane masks were equal. This caused issues because the same hardware register can be expressed in multiple ways, for example (on x86) AX + some_mask could be AL, but also AL by itself would be AL, while at the same the direct comparison would indicate that they’re different. The comparison would become more meaningful if we could try to convert the RegisterRefs so that the register ids were the same. This is what the mapTo function was helping with. This is no longer necessary to do, but I haven’t gotten around to changing that part of the code yet.