[llvm] r205988 - [ARM64, C++11]: Range'ify loops in the conditional-compare pass.

Jim Grosbach grosbach at apple.com
Thu Apr 10 16:16:28 PDT 2014


On Apr 10, 2014, at 4:10 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Apr 10, 2014 at 3:57 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> On Apr 10, 2014, at 3:14 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> On Thu, Apr 10, 2014 at 2:49 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> Author: grosbach
>> Date: Thu Apr 10 16:49:24 2014
>> New Revision: 205988
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=205988&view=rev
>> Log:
>> [ARM64,C++11]: Range'ify loops in the conditional-compare pass.
>> 
>> Modified:
>>   llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp
>> 
>> Modified: llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp?rev=205988&r1=205987&r2=205988&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp Thu Apr 10
>> 16:49:24 2014
>> @@ -212,13 +212,14 @@ public:
>> // Check that all PHIs in Tail are selecting the same value from Head and
>> CmpBB.
>> // This means that no if-conversion is required when merging CmpBB into
>> Head.
>> bool SSACCmpConv::trivialTailPHIs() {
>> -  for (MachineBasicBlock::iterator I = Tail->begin(), E = Tail->end();
>> -       I != E && I->isPHI(); ++I) {
>> +  for (auto &I : *Tail) {
>> +    if (!I.isPHI())
>> +      break;
>>    unsigned HeadReg = 0, CmpBBReg = 0;
>>    // PHI operands come in (VReg, MBB) pairs.
>> -    for (unsigned oi = 1, oe = I->getNumOperands(); oi != oe; oi += 2) {
>> -      MachineBasicBlock *MBB = I->getOperand(oi + 1).getMBB();
>> -      unsigned Reg = I->getOperand(oi).getReg();
>> +    for (unsigned oi = 1, oe = I.getNumOperands(); oi != oe; oi += 2) {
>> +      MachineBasicBlock *MBB = I.getOperand(oi + 1).getMBB();
>> +      unsigned Reg = I.getOperand(oi).getReg();
>>      if (MBB == Head) {
>>        assert((!HeadReg || HeadReg == Reg) && "Inconsistent PHI operands");
>>        HeadReg = Reg;
>> @@ -237,14 +238,15 @@ bool SSACCmpConv::trivialTailPHIs() {
>> // Assuming that trivialTailPHIs() is true, update the Tail PHIs by simply
>> // removing the CmpBB operands. The Head operands will be identical.
>> void SSACCmpConv::updateTailPHIs() {
>> -  for (MachineBasicBlock::iterator I = Tail->begin(), E = Tail->end();
>> -       I != E && I->isPHI(); ++I) {
>> +  for (auto &I : *Tail) {
>> +    if (!I.isPHI())
>> +      break;
>>    // I is a PHI. It can have multiple entries for CmpBB.
>> -    for (unsigned oi = I->getNumOperands(); oi > 2; oi -= 2) {
>> +    for (unsigned oi = I.getNumOperands(); oi > 2; oi -= 2) {
>>      // PHI operands are (Reg, MBB) at (oi-2, oi-1).
>> -      if (I->getOperand(oi - 1).getMBB() == CmpBB) {
>> -        I->RemoveOperand(oi - 1);
>> -        I->RemoveOperand(oi - 2);
>> +      if (I.getOperand(oi - 1).getMBB() == CmpBB) {
>> +        I.RemoveOperand(oi - 1);
>> +        I.RemoveOperand(oi - 2);
>>      }
>>    }
>>  }
>> @@ -387,10 +389,8 @@ bool SSACCmpConv::canSpeculateInstrs(Mac
>> 
>>  // Check all instructions, except the terminators. It is assumed that
>>  // terminators never have side effects or define any used register values.
>> -  for (MachineBasicBlock::iterator I = MBB->begin(),
>> -                                   E = MBB->getFirstTerminator();
>> -       I != E; ++I) {
>> -    if (I->isDebugValue())
>> +  for (auto &I : make_range(MBB->begin(), MBB->getFirstTerminator())) {
>> +    if (I.isDebugValue())
>>      continue;
>> 
>>    if (++InstrCount > BlockInstrLimit && !Stress) {
>> @@ -400,29 +400,29 @@ bool SSACCmpConv::canSpeculateInstrs(Mac
>>    }
>> 
>>    // There shouldn't normally be any phis in a single-predecessor block.
>> -    if (I->isPHI()) {
>> -      DEBUG(dbgs() << "Can't hoist: " << *I);
>> +    if (I.isPHI()) {
>> +      DEBUG(dbgs() << "Can't hoist: " << I);
>>      return false;
>>    }
>> 
>>    // Don't speculate loads. Note that it may be possible and desirable to
>>    // speculate GOT or constant pool loads that are guaranteed not to trap,
>>    // but we don't support that for now.
>> -    if (I->mayLoad()) {
>> -      DEBUG(dbgs() << "Won't speculate load: " << *I);
>> +    if (I.mayLoad()) {
>> +      DEBUG(dbgs() << "Won't speculate load: " << I);
>>      return false;
>>    }
>> 
>>    // We never speculate stores, so an AA pointer isn't necessary.
>>    bool DontMoveAcrossStore = true;
>> -    if (!I->isSafeToMove(TII, 0, DontMoveAcrossStore)) {
>> -      DEBUG(dbgs() << "Can't speculate: " << *I);
>> +    if (!I.isSafeToMove(TII, 0, DontMoveAcrossStore)) {
>> +      DEBUG(dbgs() << "Can't speculate: " << I);
>>      return false;
>>    }
>> 
>>    // Only CmpMI is allowed to clobber the flags.
>> -    if (&*I != CmpMI && I->modifiesRegister(ARM64::CPSR, TRI)) {
>> -      DEBUG(dbgs() << "Clobbers flags: " << *I);
>> +    if (&I != CmpMI && I.modifiesRegister(ARM64::CPSR, TRI)) {
>> +      DEBUG(dbgs() << "Clobbers flags: " << I);
>>      return false;
>>    }
>>  }
>> @@ -906,11 +906,9 @@ bool ARM64ConditionalCompares::runOnMach
>>  // Visit blocks in dominator tree pre-order. The pre-order enables
>> multiple
>>  // cmp-conversions from the same head block.
>>  // Note that updateDomTree() modifies the children of the DomTree node
>> -  // currently being visited. The df_iterator supports that, it doesn't
>> look at
>> +  // currently being visited. The df_iterator supports that; it doesn't
>> look at
>>  // child_begin() / child_end() until after a node has been visited.
>> -  for (df_iterator<MachineDominatorTree *> I = df_begin(DomTree),
>> -                                           E = df_end(DomTree);
>> -       I != E; ++I)
>> +  for (auto *I : make_range(df_begin(DomTree), df_end(DomTree)))
>> 
>> 
>> Might be nice to add the range function for these two utilities
>> (df_end/begin - either "dfs" or "df_range" or something more verbose
>> like "dom_frontiers" (at a guess)?)? (I guess this is why you added
>> make_range, though - I'd assumed you were just using it to simplify
>> the implementation of range accessor functions - I'd have a slight
>> preference of avoiding its use more generally in favor of providing
>> reusable range accessors or other utilities (range
>> splitting/merging/etc rather than doing iterator stuff))
>> 
>> 
>> Well, I added it for the bits that were iterating over instructions in a
>> block but stopping at the first terminator instruction rather than end().
> 
> Ah, looking back at the change I see those now (hadn't spotted them
> amongst the code changes to the loop bodies).
> 
> Compare this to my change to MachineInstr (
> http://llvm.org/viewvc/llvm-project?rev=205680&view=rev ) - if it's
> common to need "all the instructions except the terminators" it might
> be nice to introduce an accessor for that rather than building it at
> every loop. Maybe "non_terminators()". Indeed in time we might be able
> to get rid of "getFirstTerminator" and just have "terminators()",
> "non_terminators()" and "instructions()" (the latter being terminators
> + non_terminators).

Agreed. I’m just working through these as I go. The plan is to start w/ one-off uses w/ make_range() and then convert those to actual accessor functions like you’re suggesting when/if I encounter more than a handful of instances of a given pattern. Sounds like that may be the case for this one and I like the general direction. I’ll have a look, thanks!


> Just ideas, this stuff is all still pretty experimental as to what the
> "right" usage is.

Yep, totally. Spurring some of that discussion is exactly what I’m hoping to get out of making these little changes.

-Jim

> 
>> Here I just got excited about having make_range() and used it. I have a
>> hammer now, so everything's a nail.
>> 
>> Actual functions for these seems reasonable. I'll add those in a moment.
>> 
>> 
>>    if (tryConvert(I->getBlock()))
>>      Changed = true;
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140410/e42f6edb/attachment.html>


More information about the llvm-commits mailing list