[PATCH] D54128: Fix MachineInstr::findRegisterUseOperandIdx subreg checks

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/CodeGen/MachineInstr.cpp:936
       continue;
-    if (MOReg == Reg || (TRI && TargetRegisterInfo::isPhysicalRegister(MOReg) &&
-                         TargetRegisterInfo::isPhysicalRegister(Reg) &&
-                         TRI->isSubRegister(MOReg, Reg)))
+    if (MOReg == Reg || (TRI && Reg && MOReg && TRI->regsOverlap(MOReg, Reg)))
       if (!isKill || MO.isKill())
----------------
rampitec wrote:
> arsenm wrote:
> > Why is TRI optional here? I would expect it to be mandatory if there is a physical register and assert
> That's a good question why, but it is optional.
> I have tried to use just TRI->regsOverlap() instead of the whole condition, and then run into crashes due to TRI being nullptr.
Here are just some examples of places where the function is called without optional TRI operand and with a physreg:

lib/Target/AArch64/AArch64InstrInfo.cpp:

1515│   case AArch64::FCSELDrrr: {
1516├>    int Idx = Instr.findRegisterUseOperandIdx(AArch64::NZCV);

lib/Target/AMDGPU/SIInstrInfo.cpp:

4769├>    if (MI.findRegisterUseOperandIdx(AMDGPU::SCC) != -1)
4770│       Worklist.insert(&MI);

lib/Target/ARM/ARMLoadStoreOptimizer.cpp:

 891├>  bool BaseKill = LatestMI->killsRegister(Base);

and then:

include/llvm/CodeGen/MachineInstr.h:

1134│   bool killsRegister(unsigned Reg,
1135│                      const TargetRegisterInfo *TRI = nullptr) const {
1136├>    return findRegisterUseOperandIdx(Reg, true, TRI) != -1;

There is huge number of places across all targets.


https://reviews.llvm.org/D54128