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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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:
> > 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.
Is there any real reason not to pass it in though? I would think it shouldn't be hard to make it mandatory, other than the manual work


https://reviews.llvm.org/D54128