[scan-build] fix dead store warnings emitted on LLVM Hexagon code base

[apelete]

Summary

This patch fixes a logic error warning of the type "assigned value is
garbage or undefined" reported by Clang Static Analyzer on the
following file:

• lib/Target/Hexagon/HexagonOptAddrMode.cpp.

It also addresses dead store warnings of the type "dead assignment" on
the following files:

• lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp,
• lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp,
• lib/Target/Hexagon/HexagonFixupHwLoops.cpp,
• lib/Target/Hexagon/HexagonCommonGEP.cpp.

Signed-off-by: Apelete Seketeli <[e-mail]>

[kparzysz]

lib/Target/Hexagon/HexagonOptAddrMode.cpp

399 ↗

(On Diff #56093)

This is not strictly necessary since OpStart is only used when Changed it "true". That happens iff OpStart is assigned a value.

[apelete]

lib/Target/Hexagon/HexagonOptAddrMode.cpp

399 ↗

(On Diff #56093)

Not strictly necessary, it just helps suppressing a logic error warning, I agree.
To be more precise, in the following code path:

406. if (ImmOpNum == 0) {
407. if (HII->getAddrMode(OldMI) == HexagonII::BaseRegOffset) {

...

415. OpStart = 4;
416. } else if (HII->getAddrMode(OldMI) == HexagonII::BaseImmOffset) {

...

424. OpStart = 3;
425. }
426. Changed = true;

...

429. } else if (ImmOpNum == 1 && OldMI->getOperand(2).getImm() == 0) {

...

440. }

From a static analysis point of view, if the branches at line 407 and 416 evaluate to 'false', then OpStart would be left un-initialized.
Then, when "Changed" is 'true':

441. if (Changed)
442. for (unsigned i = OpStart; i < OpEnd; ++i)
443. MIB.addOperand(OldMI->getOperand(i));

'i' would be assigned the value of 'OpStart' which hasn't been initialized, thus the warning "assigned value is
garbage or undefined" from a static analysis point of view.

I understand that exact code path is unlikely to be taken at runtime.
Would you prefer we ignore the warning then (instead of trying to suppress it) ?

[kparzysz]

You're right. Maybe the best thing to do would be to initialize OpStart to OpEnd, and add an assert:

if (Changed) {
  assert(OpStart < OpEnd);
  for (...) ...
}

[apelete]

[scan-build] fix dead store warnings emitted on LLVM Hexagon code base

Changes since last revision:

• lib/Target/Hexagon/HexagonOptAddrMode.cpp: initialize 'OpStart' with 'OpEnd' value and assert if number of operands has not been computed before changing instruction store.

[kparzysz]

LGTM. Thanks!

[apelete]

[scan-build] fix dead store warnings emitted on LLVM Hexagon code base

• rebased patch on trunk and validated against test suite,

• lib/Target/Hexagon/HexagonOptAddrMode.cpp: removed changes as they fail to pass the following tests:
  • LLVM :: CodeGen/Hexagon/opt-addr-mode.ll,
  • LLVM :: CodeGen/Hexagon/zextloadi1.ll.

[apelete]

In D19900#421341, @kparzysz wrote:

LGTM. Thanks!

Removed some changes in last revision since they fail to pass some tests, will look into it when I get the time.
If current changes are good for you please go ahead and commit this for me when you're ready as I don't have commit access.

Thanks.

llvm/trunk/lib/Target/Hexagon/HexagonCommonGEP.cpp

if (GN.Flags & GepNode::Internal) {
         OS << ',';
       OS << "internal";
       Comma = true;
     }
     if (GN.Flags & GepNode::Used) {
       if (Comma)
         OS << ',';
       OS << "used";
-      Comma = true;
     }
     OS << "} ";
     if (GN.Flags & GepNode::Root)
       OS << "BaseVal:" << GN.BaseVal->getName() << '(' << GN.BaseVal << ')';
     else
       OS << "Parent:" << GN.Parent;
 
     OS << " Idx:";

llvm/trunk/lib/Target/Hexagon/HexagonFixupHwLoops.cpp

for (const MachineBasicBlock &MBB : MF) {
 
     BlockToInstOffset[&MBB] = InstOffset;
     for (const MachineInstr &MI : MBB)
       InstOffset += HII->getSize(&MI);
   }
 
   // Second pass - check each loop instruction to see if it needs to be
   // converted.
-  InstOffset = 0;
   bool Changed = false;
   for (MachineBasicBlock &MBB : MF) {
     InstOffset = BlockToInstOffset[&MBB];
 
     // Loop over all the instructions.
     MachineBasicBlock::iterator MII = MBB.begin();
     MachineBasicBlock::iterator MIE = MBB.end();
     while (MII != MIE) {

llvm/trunk/lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp

if (HexagonMCInstrInfo::isInnerLoop(*MI)) {
     OS << Separator;
     Separator = " ";
     MCInst ME;
     ME.setOpcode(Hexagon::ENDLOOP0);
     printInstruction(&ME, OS);
   }
   if (HexagonMCInstrInfo::isOuterLoop(*MI)) {
     OS << Separator;
-    Separator = " ";
     MCInst ME;
     ME.setOpcode(Hexagon::ENDLOOP1);
     printInstruction(&ME, OS);
   }
 }
 
 void HexagonInstPrinter::printOperand(MCInst const *MI, unsigned OpNo,
                                       raw_ostream &O) const {

llvm/trunk/lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp

while (possibleDuplexes.size() > 0 && (!doneShuffling)) {
     // case of Duplex Found
     DuplexCandidate duplexToTry = possibleDuplexes.pop_back_val();
     MCInst Attempt(MCB);
     HexagonMCInstrInfo::replaceDuplex(Context, Attempt, duplexToTry);
     HexagonMCShuffler MCS(MCII, STI, Attempt); // copy packet to the shuffler
     if (MCS.size() == 1) {                     // case of one duplex
       // copy the created duplex in the shuffler to the bundle
       MCS.copyTo(MCB);
-      doneShuffling = true;
       return HexagonShuffler::SHUFFLE_SUCCESS;
     }
     // try shuffle with this duplex
     doneShuffling = MCS.reshuffleTo(MCB);
     shuffleError = MCS.getError();
 
     if (doneShuffling)
       break;