[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())
----------------
arsenm wrote:
> 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
I do not think there is such a reason. That is just a lot of places across all backends.
Anyway, this is really orthogonal to this patch.


https://reviews.llvm.org/D54128