[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:
> 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.
Altogether 2539 tests fail across all targets if TRI check is removed, even if physreg check is in place.


https://reviews.llvm.org/D54128