[llvm] r275141 - Mips: Avoid implicit iterator conversions, NFC
Duncan Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 15 07:13:52 PDT 2016
Hmm. Thanks for the revert and sorry for the trouble.
Please send a reproduction so I can investigate (assuming you can confirm this is the cause). Likely this is a surprise end iterator being dereferenced somehow (I saw some of those in other patches, but caught the problem locally in the lit tests).
-- dpnes
> On Jul 15, 2016, at 07:02, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
>
> Hi,
>
> This commit was identified by a bisect as causing some test failures in https://dmz-portal.mips.com/bb/builders/LLVM%20with%20integrated%20assembler%20and%20fPIC%20and%20-O0 and a few other buildbots. I don't see an obvious reason for this in the patch but I've reverted it in r275562. If these buildbots turn green afterwards then there must be some kind of functional difference in here.
>
>> -----Original Message-----
>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf
>> Of Duncan P. N. Exon Smith via llvm-commits
>> Sent: 12 July 2016 02:47
>> To: llvm-commits at lists.llvm.org
>> Subject: [llvm] r275141 - Mips: Avoid implicit iterator conversions, NFC
>>
>> Author: dexonsmith
>> Date: Mon Jul 11 20:47:02 2016
>> New Revision: 275141
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=275141&view=rev
>> Log:
>> Mips: Avoid implicit iterator conversions, NFC
>>
>> Avoid implicit conversions from MachineInstrBundleIterator to
>> MachineInstr* in the Mips backend, mainly by preferring MachineInstr&
>> over MachineInstr* when a pointer isn't nullable and using range-based
>> for loops.
>>
>> Modified:
>> llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp
>> llvm/trunk/lib/Target/Mips/MipsDelaySlotFiller.cpp
>> llvm/trunk/lib/Target/Mips/MipsHazardSchedule.cpp
>> llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp
>> llvm/trunk/lib/Target/Mips/MipsInstrInfo.h
>> llvm/trunk/lib/Target/Mips/MipsLongBranch.cpp
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp?rev=27514
>> 1&r1=275140&r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsConstantIslandPass.cpp Mon Jul 11
>> 20:47:02 2016
>> @@ -369,7 +369,7 @@ namespace {
>>
>> void doInitialPlacement(std::vector<MachineInstr*> &CPEMIs);
>> CPEntry *findConstPoolEntry(unsigned CPI, const MachineInstr *CPEMI);
>> - unsigned getCPELogAlign(const MachineInstr *CPEMI);
>> + unsigned getCPELogAlign(const MachineInstr &CPEMI);
>> void initializeFunctionInfo(const std::vector<MachineInstr*> &CPEMIs);
>> unsigned getOffsetOf(MachineInstr *MI) const;
>> unsigned getUserOffset(CPUser&) const;
>> @@ -381,7 +381,7 @@ namespace {
>> const CPUser &U);
>>
>> void computeBlockSize(MachineBasicBlock *MBB);
>> - MachineBasicBlock *splitBlockBeforeInstr(MachineInstr *MI);
>> + MachineBasicBlock *splitBlockBeforeInstr(MachineInstr &MI);
>> void updateForInsertedWaterBlock(MachineBasicBlock *NewBB);
>> void adjustBBOffsetsAfter(MachineBasicBlock *BB);
>> bool decrementCPEReferenceCount(unsigned CPI, MachineInstr* CPEMI);
>> @@ -620,14 +620,14 @@ MipsConstantIslands::CPEntry
>>
>> /// getCPELogAlign - Returns the required alignment of the constant pool
>> entry
>> /// represented by CPEMI. Alignment is measured in log2(bytes) units.
>> -unsigned MipsConstantIslands::getCPELogAlign(const MachineInstr
>> *CPEMI) {
>> - assert(CPEMI && CPEMI->getOpcode() == Mips::CONSTPOOL_ENTRY);
>> +unsigned MipsConstantIslands::getCPELogAlign(const MachineInstr
>> &CPEMI) {
>> + assert(CPEMI.getOpcode() == Mips::CONSTPOOL_ENTRY);
>>
>> // Everything is 4-byte aligned unless AlignConstantIslands is set.
>> if (!AlignConstantIslands)
>> return 2;
>>
>> - unsigned CPI = CPEMI->getOperand(1).getIndex();
>> + unsigned CPI = CPEMI.getOperand(1).getIndex();
>> assert(CPI < MCP->getConstants().size() && "Invalid constant pool index.");
>> unsigned Align = MCP->getConstants()[CPI].getAlignment();
>> assert(isPowerOf2_32(Align) && "Invalid CPE alignment");
>> @@ -654,21 +654,17 @@ initializeFunctionInfo(const std::vector
>> adjustBBOffsetsAfter(&MF->front());
>>
>> // Now go back through the instructions and build up our data structures.
>> - for (MachineFunction::iterator MBBI = MF->begin(), E = MF->end();
>> - MBBI != E; ++MBBI) {
>> - MachineBasicBlock &MBB = *MBBI;
>> -
>> + for (MachineBasicBlock &MBB : *MF) {
>> // If this block doesn't fall through into the next MBB, then this is
>> // 'water' that a constant pool island could be placed.
>> if (!BBHasFallthrough(&MBB))
>> WaterList.push_back(&MBB);
>> - for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
>> - I != E; ++I) {
>> - if (I->isDebugValue())
>> + for (MachineInstr &MI : MBB) {
>> + if (MI.isDebugValue())
>> continue;
>>
>> - int Opc = I->getOpcode();
>> - if (I->isBranch()) {
>> + int Opc = MI.getOpcode();
>> + if (MI.isBranch()) {
>> bool isCond = false;
>> unsigned Bits = 0;
>> unsigned Scale = 1;
>> @@ -737,7 +733,7 @@ initializeFunctionInfo(const std::vector
>> }
>> // Record this immediate branch.
>> unsigned MaxOffs = ((1 << (Bits-1))-1) * Scale;
>> - ImmBranches.push_back(ImmBranch(I, MaxOffs, isCond, UOpc));
>> + ImmBranches.push_back(ImmBranch(&MI, MaxOffs, isCond, UOpc));
>> }
>>
>> if (Opc == Mips::CONSTPOOL_ENTRY)
>> @@ -745,8 +741,8 @@ initializeFunctionInfo(const std::vector
>>
>>
>> // Scan the instructions for constant pool operands.
>> - for (unsigned op = 0, e = I->getNumOperands(); op != e; ++op)
>> - if (I->getOperand(op).isCPI()) {
>> + for (unsigned op = 0, e = MI.getNumOperands(); op != e; ++op)
>> + if (MI.getOperand(op).isCPI()) {
>>
>> // We found one. The addressing mode tells us the max displacement
>> // from the PC that this instruction permits.
>> @@ -775,12 +771,12 @@ initializeFunctionInfo(const std::vector
>> break;
>> }
>> // Remember that this is a user of a CP entry.
>> - unsigned CPI = I->getOperand(op).getIndex();
>> + unsigned CPI = MI.getOperand(op).getIndex();
>> MachineInstr *CPEMI = CPEMIs[CPI];
>> unsigned MaxOffs = ((1 << Bits)-1) * Scale;
>> unsigned LongFormMaxOffs = ((1 << LongFormBits)-1) *
>> LongFormScale;
>> - CPUsers.push_back(CPUser(I, CPEMI, MaxOffs, NegOk,
>> - LongFormMaxOffs, LongFormOpcode));
>> + CPUsers.push_back(CPUser(&MI, CPEMI, MaxOffs, NegOk,
>> LongFormMaxOffs,
>> + LongFormOpcode));
>>
>> // Increment corresponding CPEntry reference count.
>> CPEntry *CPE = findConstPoolEntry(CPI, CPEMI);
>> @@ -804,10 +800,8 @@ void MipsConstantIslands::computeBlockSi
>> BasicBlockInfo &BBI = BBInfo[MBB->getNumber()];
>> BBI.Size = 0;
>>
>> - for (MachineBasicBlock::iterator I = MBB->begin(), E = MBB->end(); I != E;
>> - ++I)
>> - BBI.Size += TII->GetInstSizeInBytes(I);
>> -
>> + for (const MachineInstr &MI : *MBB)
>> + BBI.Size += TII->GetInstSizeInBytes(MI);
>> }
>>
>> /// getOffsetOf - Return the current offset of the specified machine
>> instruction
>> @@ -824,7 +818,7 @@ unsigned MipsConstantIslands::getOffsetO
>> // Sum instructions before MI in MBB.
>> for (MachineBasicBlock::iterator I = MBB->begin(); &*I != MI; ++I) {
>> assert(I != MBB->end() && "Didn't find MI in its own basic block?");
>> - Offset += TII->GetInstSizeInBytes(I);
>> + Offset += TII->GetInstSizeInBytes(*I);
>> }
>> return Offset;
>> }
>> @@ -863,9 +857,9 @@ unsigned MipsConstantIslands::getUserOff
>> /// Split the basic block containing MI into two blocks, which are joined by
>> /// an unconditional branch. Update data structures and renumber blocks to
>> /// account for this change and returns the newly created block.
>> -MachineBasicBlock *MipsConstantIslands::splitBlockBeforeInstr
>> - (MachineInstr *MI) {
>> - MachineBasicBlock *OrigBB = MI->getParent();
>> +MachineBasicBlock *
>> +MipsConstantIslands::splitBlockBeforeInstr(MachineInstr &MI) {
>> + MachineBasicBlock *OrigBB = MI.getParent();
>>
>> // Create a new MBB for the code after the OrigBB.
>> MachineBasicBlock *NewBB =
>> @@ -955,7 +949,7 @@ bool MipsConstantIslands::isOffsetInRang
>> bool MipsConstantIslands::isWaterInRange(unsigned UserOffset,
>> MachineBasicBlock* Water, CPUser &U,
>> unsigned &Growth) {
>> - unsigned CPELogAlign = getCPELogAlign(U.CPEMI);
>> + unsigned CPELogAlign = getCPELogAlign(*U.CPEMI);
>> unsigned CPEOffset = BBInfo[Water-
>>> getNumber()].postOffset(CPELogAlign);
>> unsigned NextBlockOffset, NextBlockAlignment;
>> MachineFunction::const_iterator NextBlock = ++Water->getIterator();
>> @@ -1237,7 +1231,7 @@ void MipsConstantIslands::createNewWater
>> CPUser &U = CPUsers[CPUserIndex];
>> MachineInstr *UserMI = U.MI;
>> MachineInstr *CPEMI = U.CPEMI;
>> - unsigned CPELogAlign = getCPELogAlign(CPEMI);
>> + unsigned CPELogAlign = getCPELogAlign(*CPEMI);
>> MachineBasicBlock *UserMBB = UserMI->getParent();
>> const BasicBlockInfo &UserBBI = BBInfo[UserMBB->getNumber()];
>>
>> @@ -1303,11 +1297,12 @@ void MipsConstantIslands::createNewWater
>> unsigned CPUIndex = CPUserIndex+1;
>> unsigned NumCPUsers = CPUsers.size();
>> //MachineInstr *LastIT = 0;
>> - for (unsigned Offset = UserOffset+TII->GetInstSizeInBytes(UserMI);
>> + for (unsigned Offset = UserOffset + TII->GetInstSizeInBytes(*UserMI);
>> Offset < BaseInsertOffset;
>> - Offset += TII->GetInstSizeInBytes(MI), MI = std::next(MI)) {
>> + Offset += TII->GetInstSizeInBytes(*MI), MI = std::next(MI)) {
>> assert(MI != UserMBB->end() && "Fell off end of block");
>> - if (CPUIndex < NumCPUsers && CPUsers[CPUIndex].MI == MI) {
>> + if (CPUIndex < NumCPUsers &&
>> + CPUsers[CPUIndex].MI == static_cast<MachineInstr *>(MI)) {
>> CPUser &U = CPUsers[CPUIndex];
>> if (!isOffsetInRange(Offset, EndInsertOffset, U)) {
>> // Shift intertion point by one unit of alignment so it is within reach.
>> @@ -1323,8 +1318,7 @@ void MipsConstantIslands::createNewWater
>> }
>> }
>>
>> - --MI;
>> - NewMBB = splitBlockBeforeInstr(MI);
>> + NewMBB = splitBlockBeforeInstr(*--MI);
>> }
>>
>> /// handleConstantPoolUser - Analyze the specified user, checking to see if
>> it
>> @@ -1417,7 +1411,7 @@ bool MipsConstantIslands::handleConstant
>> ++NumCPEs;
>>
>> // Mark the basic block as aligned as required by the const-pool entry.
>> - NewIsland->setAlignment(getCPELogAlign(U.CPEMI));
>> + NewIsland->setAlignment(getCPELogAlign(*U.CPEMI));
>>
>> // Increase the size of the island block to account for the new entry.
>> BBInfo[NewIsland->getNumber()].Size += Size;
>> @@ -1451,7 +1445,7 @@ void MipsConstantIslands::removeDeadCPEM
>> CPEBB->setAlignment(0);
>> } else
>> // Entries are sorted by descending alignment, so realign from the front.
>> - CPEBB->setAlignment(getCPELogAlign(CPEBB->begin()));
>> + CPEBB->setAlignment(getCPELogAlign(*CPEBB->begin()));
>>
>> adjustBBOffsetsAfter(CPEBB);
>> // An island has only one predecessor BB and one successor BB. Check if
>> @@ -1625,10 +1619,10 @@ MipsConstantIslands::fixupConditionalBr(
>>
>>
>> if (NeedSplit) {
>> - splitBlockBeforeInstr(MI);
>> + splitBlockBeforeInstr(*MI);
>> // No need for the branch to the next block. We're adding an
>> unconditional
>> // branch to the destination.
>> - int delta = TII->GetInstSizeInBytes(&MBB->back());
>> + int delta = TII->GetInstSizeInBytes(MBB->back());
>> BBInfo[MBB->getNumber()].Size -= delta;
>> MBB->back().eraseFromParent();
>> // BBInfo[SplitBB].Offset is wrong temporarily, fixed below
>> @@ -1650,14 +1644,14 @@ MipsConstantIslands::fixupConditionalBr(
>> .addMBB(NextBB);
>> }
>> Br.MI = &MBB->back();
>> - BBInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(&MBB-
>>> back());
>> + BBInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(MBB-
>>> back());
>> BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr)).addMBB(DestBB);
>> - BBInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(&MBB-
>>> back());
>> + BBInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(MBB-
>>> back());
>> unsigned MaxDisp = getUnconditionalBrDisp(Br.UncondBr);
>> ImmBranches.push_back(ImmBranch(&MBB->back(), MaxDisp, false,
>> Br.UncondBr));
>>
>> // Remove the old conditional branch. It may or may not still be in MBB.
>> - BBInfo[MI->getParent()->getNumber()].Size -= TII-
>>> GetInstSizeInBytes(MI);
>> + BBInfo[MI->getParent()->getNumber()].Size -= TII-
>>> GetInstSizeInBytes(*MI);
>> MI->eraseFromParent();
>> adjustBBOffsetsAfter(MBB);
>> return true;
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsDelaySlotFiller.cpp
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsDelaySlotFiller.cpp?rev=275141&r1=
>> 275140&r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsDelaySlotFiller.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsDelaySlotFiller.cpp Mon Jul 11 20:47:02
>> 2016
>> @@ -600,9 +600,9 @@ bool Filler::runOnMachineBasicBlock(Mach
>>
>> if (Filled) {
>> // Get instruction with delay slot.
>> - MachineBasicBlock::instr_iterator DSI(I);
>> + MachineBasicBlock::instr_iterator DSI = I.getInstrIterator();
>>
>> - if (InMicroMipsMode && TII->GetInstSizeInBytes(&*std::next(DSI)) ==
>> 2 &&
>> + if (InMicroMipsMode && TII->GetInstSizeInBytes(*std::next(DSI)) == 2
>> &&
>> DSI->isCall()) {
>> // If instruction in delay slot is 16b change opcode to
>> // corresponding instruction with short delay slot.
>> @@ -692,7 +692,7 @@ bool Filler::searchRange(MachineBasicBlo
>> bool InMicroMipsMode = STI.inMicroMipsMode();
>> const MipsInstrInfo *TII = STI.getInstrInfo();
>> unsigned Opcode = (*Slot).getOpcode();
>> - if (InMicroMipsMode && TII->GetInstSizeInBytes(&(*CurrI)) == 2 &&
>> + if (InMicroMipsMode && TII->GetInstSizeInBytes(*CurrI) == 2 &&
>> (Opcode == Mips::JR || Opcode == Mips::PseudoIndirectBranch ||
>> Opcode == Mips::PseudoReturn))
>> continue;
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsHazardSchedule.cpp
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsHazardSchedule.cpp?rev=275141&r
>> 1=275140&r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsHazardSchedule.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsHazardSchedule.cpp Mon Jul 11
>> 20:47:02 2016
>> @@ -137,8 +137,8 @@ bool MipsHazardSchedule::runOnMachineFun
>>
>> if (InsertNop) {
>> Changed = true;
>> - MIBundleBuilder(I)
>> - .append(BuildMI(MF, I->getDebugLoc(), TII->get(Mips::NOP)));
>> + MIBundleBuilder(&*I).append(
>> + BuildMI(MF, I->getDebugLoc(), TII->get(Mips::NOP)));
>> NumInsertedNops++;
>> }
>> }
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp?rev=275141&r1=2751
>> 40&r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsInstrInfo.cpp Mon Jul 11 20:47:02 2016
>> @@ -377,19 +377,19 @@ bool MipsInstrInfo::HasForbiddenSlot(con
>> }
>>
>> /// Return the number of bytes of code the specified instruction may be.
>> -unsigned MipsInstrInfo::GetInstSizeInBytes(const MachineInstr *MI) const {
>> - switch (MI->getOpcode()) {
>> +unsigned MipsInstrInfo::GetInstSizeInBytes(const MachineInstr &MI) const
>> {
>> + switch (MI.getOpcode()) {
>> default:
>> - return MI->getDesc().getSize();
>> + return MI.getDesc().getSize();
>> case TargetOpcode::INLINEASM: { // Inline Asm: Variable size.
>> - const MachineFunction *MF = MI->getParent()->getParent();
>> - const char *AsmStr = MI->getOperand(0).getSymbolName();
>> + const MachineFunction *MF = MI.getParent()->getParent();
>> + const char *AsmStr = MI.getOperand(0).getSymbolName();
>> return getInlineAsmLength(AsmStr, *MF->getTarget().getMCAsmInfo());
>> }
>> case Mips::CONSTPOOL_ENTRY:
>> // If this machine instr is a constant pool entry, its size is recorded as
>> // operand #2.
>> - return MI->getOperand(2).getImm();
>> + return MI.getOperand(2).getImm();
>> }
>> }
>>
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsInstrInfo.h
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsInstrInfo.h?rev=275141&r1=275140
>> &r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsInstrInfo.h (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsInstrInfo.h Mon Jul 11 20:47:02 2016
>> @@ -92,7 +92,7 @@ public:
>> virtual unsigned getOppositeBranchOpc(unsigned Opc) const = 0;
>>
>> /// Return the number of bytes of code the specified instruction may be.
>> - unsigned GetInstSizeInBytes(const MachineInstr *MI) const;
>> + unsigned GetInstSizeInBytes(const MachineInstr &MI) const;
>>
>> void storeRegToStackSlot(MachineBasicBlock &MBB,
>> MachineBasicBlock::iterator MBBI,
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsLongBranch.cpp
>> URL: http://llvm.org/viewvc/llvm-
>> project/llvm/trunk/lib/Target/Mips/MipsLongBranch.cpp?rev=275141&r1=27
>> 5140&r2=275141&view=diff
>> ==========================================================
>> ====================
>> --- llvm/trunk/lib/Target/Mips/MipsLongBranch.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsLongBranch.cpp Mon Jul 11 20:47:02
>> 2016
>> @@ -179,7 +179,7 @@ void MipsLongBranch::initMBBInfo() {
>> // Compute size of MBB.
>> for (MachineBasicBlock::instr_iterator MI = MBB->instr_begin();
>> MI != MBB->instr_end(); ++MI)
>> - MBBInfos[I].Size += TII->GetInstSizeInBytes(&*MI);
>> + MBBInfos[I].Size += TII->GetInstSizeInBytes(*MI);
>>
>> // Search for MBB's branch instruction.
>> ReverseIter End = MBB->rend();
>> @@ -187,7 +187,7 @@ void MipsLongBranch::initMBBInfo() {
>>
>> if ((Br != End) && !Br->isIndirectBranch() &&
>> (Br->isConditionalBranch() || (Br->isUnconditionalBranch() && IsPIC)))
>> - MBBInfos[I].Br = (++Br).base();
>> + MBBInfos[I].Br = &*(++Br).base();
>> }
>> }
>>
>> @@ -241,7 +241,7 @@ void MipsLongBranch::replaceBranch(Machi
>> // Bundle the instruction in the delay slot to the newly created branch
>> // and erase the original branch.
>> assert(Br->isBundledWithSucc());
>> - MachineBasicBlock::instr_iterator II(Br);
>> + MachineBasicBlock::instr_iterator II = Br.getInstrIterator();
>> MIBundleBuilder(&*MIB).append((++II)->removeFromBundle());
>> }
>> Br->eraseFromParent();
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list