[X86] Bugfix for nullptr check by klocwork

[xiangzhangllvm]

Summary

klocwork critical issues in CG files:

#10087: Pointer 'ReferenceDI' returned from call to function 'get' at line 87 may be NULL and will be dereferenced at line 90. llvm/lib/Target/X86/X86DiscriminateMemOps.cpp:87

#13373: Pointer 'DI' returned from call to function 'cloneWithDiscriminator' at line 145 may be NULL and will be dereferenced at line 150. llvm/lib/Target/X86/X86DiscriminateMemOps.cpp:145

#17178: Pointer 'RegRB' returned from call to function 'getRegBank' at line 1603 may be NULL and will be dereferenced at line 1604. llvm/lib/Target/X86/X86InstructionSelector.cpp:1603

[craig.topper]

lib/Target/X86/X86InstructionSelector.cpp

1606 ↗

(On Diff #187286)

Get rid of this reference variable and just use the pointer with an -> below. Rename it back to RegRB.

[xiangzhangllvm]

Ok , I'll update it like this:

1 diff --git a/lib/Target/X86/X86InstructionSelector.cpp b/lib/Target/X86/X86InstructionSelector.cpp

2 index c3c41fe..92de59b 100644
3 --- a/lib/Target/X86/X86InstructionSelector.cpp
4 +++ b/lib/Target/X86/X86InstructionSelector.cpp
5 @@ -1600,11 +1600,8 @@ bool X86InstructionSelector::selectDivRem(MachineInstr &I,
6    assert(RegTy == MRI.getType(Op1Reg) && RegTy == MRI.getType(Op2Reg) &&
7           "Arguments and return value types must match");
8
9 -  const RegisterBank *PtrRegRB = RBI.getRegBank(DstReg, MRI, TRI);

10 - if (!PtrRegRB)
11 - return false;
12 - const RegisterBank &RegRB = *PtrRegRB;
13 - if (RegRB.getID() != X86::GPRRegBankID)
14 + const RegisterBank *RegRB = RBI.getRegBank(DstReg, MRI, TRI);
15 + if (!RegRB && RegRB->getID() != X86::GPRRegBankID)
16 return false;
17
18 const static unsigned NumTypes = 4; // i8, i16, i32, i64
19 @@ -1702,7 +1699,7 @@ bool X86InstructionSelector::selectDivRem(MachineInstr &I,
20 const DivRemEntry &TypeEntry = *OpEntryIt;
21 const DivRemEntry::DivRemResult &OpEntry = TypeEntry.ResultTable[OpIndex];
22
23 - const TargetRegisterClass *RegRC = getRegClass(RegTy, RegRB);
24 + const TargetRegisterClass *RegRC = getRegClass(RegTy, *RegRB);
25 if (!RBI.constrainGenericRegister(Op1Reg, *RegRC, MRI) ||
26 !RBI.constrainGenericRegister(Op2Reg, *RegRC, MRI) ||
27 !RBI.constrainGenericRegister(DstReg, *RegRC, MRI)) {

[LuoYuanke]

lib/Target/X86/X86InstructionSelector.cpp

1604 ↗

(On Diff #187290)

Use || instead of &&?

[xiangzhangllvm]

Yes! Should be '||'! Thank you!

[craig.topper]

LGTM

llvm/trunk/lib/Target/X86/X86DiscriminateMemOps.cpp

bool X86DiscriminateMemOps::runOnMachineFunction(MachineFunction &MF) {
   DISubprogram *FDI = MF.getFunction().getSubprogram();
   if (!FDI || !FDI->getUnit()->getDebugInfoForProfiling())
     return false;
 
   // Have a default DILocation, if we find instructions with memops that don't
   // have any debug info.
   const DILocation *ReferenceDI =
       DILocation::get(FDI->getContext(), FDI->getLine(), 0, FDI);
-
+  assert(ReferenceDI && "ReferenceDI should not be nullptr");
   DenseMap<Location, unsigned> MemOpDiscriminators;
   MemOpDiscriminators[diToLocation(ReferenceDI)] = 0;
 
   // Figure out the largest discriminator issued for each Location. When we
   // issue new discriminators, we can thus avoid issuing discriminators
   // belonging to instructions that don't have memops. This isn't a requirement
   // for the goals of this pass, however, it avoids unnecessary ambiguity.
   for (auto &MBB : MF) {
for (auto &MI : MBB) {
                      << DI->getFilename() << " Line: " << DI->getLine()
                      << " Column: " << DI->getColumn()
                      << ". This is likely due to a large macro expansion. \n");
           continue;
         }
         // Since we were able to encode, bump the MemOpDiscriminators.
         ++MemOpDiscriminators[L];
         DI = DI->cloneWithDiscriminator(EncodedDiscriminator.getValue());
+        assert(DI && "DI should not be nullptr");
         updateDebugInfo(&MI, DI);
         Changed = true;
         std::pair<DenseSet<unsigned>::iterator, bool> MustInsert =
             Set.insert(DI->getBaseDiscriminator());
         (void)MustInsert; // Silence warning in release build.
         assert(MustInsert.second && "New discriminator shouldn't be present in set");
       }
 

llvm/trunk/lib/Target/X86/X86InstructionSelector.cpp

bool X86InstructionSelector::selectDivRem(MachineInstr &I,
   const unsigned DstReg = I.getOperand(0).getReg();
   const unsigned Op1Reg = I.getOperand(1).getReg();
   const unsigned Op2Reg = I.getOperand(2).getReg();
 
   const LLT RegTy = MRI.getType(DstReg);
   assert(RegTy == MRI.getType(Op1Reg) && RegTy == MRI.getType(Op2Reg) &&
          "Arguments and return value types must match");
 
-  const RegisterBank &RegRB = *RBI.getRegBank(DstReg, MRI, TRI);
-  if (RegRB.getID() != X86::GPRRegBankID)
+  const RegisterBank *RegRB = RBI.getRegBank(DstReg, MRI, TRI);
+  if (!RegRB || RegRB->getID() != X86::GPRRegBankID)
     return false;
 
   const static unsigned NumTypes = 4; // i8, i16, i32, i64
   const static unsigned NumOps = 4;   // SDiv, SRem, UDiv, URem
   const static bool S = true;         // IsSigned
   const static bool U = false;        // !IsSigned
   const static unsigned Copy = TargetOpcode::COPY;
   // For the X86 IDIV instruction, in most cases the dividend
bool X86InstructionSelector::selectDivRem(MachineInstr &I,
   case TargetOpcode::G_UREM:
     OpIndex = 3;
     break;
   }
 
   const DivRemEntry &TypeEntry = *OpEntryIt;
   const DivRemEntry::DivRemResult &OpEntry = TypeEntry.ResultTable[OpIndex];
 
-  const TargetRegisterClass *RegRC = getRegClass(RegTy, RegRB);
+  const TargetRegisterClass *RegRC = getRegClass(RegTy, *RegRB);
   if (!RBI.constrainGenericRegister(Op1Reg, *RegRC, MRI) ||
       !RBI.constrainGenericRegister(Op2Reg, *RegRC, MRI) ||
       !RBI.constrainGenericRegister(DstReg, *RegRC, MRI)) {
     LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
                       << " operand\n");
     return false;
   }