Page MenuHomePhabricator

[NFC] [PowerPC] Use isPredicable bits in instruction definitions
ClosedPublic

Authored by qiucf on Dec 27 2019, 1:28 AM.

Details

Summary

Currently, we are using method isPredicable to check opcode of machine instr on PowerPC. However, on other platforms, it's adapted to use isPredicable bit in instruction themselves. And by default, TargetInstrInfo::isPredicable(const MachineInstr&) checks the bit in instr.

This patch only affects opcodes mentioned in current PPCInstrInfo::isPredicable, to minimalize changes. We can add it to more opcodes in future patches.

Diff Detail

Event Timeline

qiucf created this revision.Dec 27 2019, 1:28 AM
qiucf updated this revision to Diff 235398.Dec 27 2019, 1:37 AM

Extend patch with full contexts.

jsji added a comment.Dec 27 2019, 6:58 AM

If this is a NFC change, please add [NFC] in summary. Or else, please add test. Thanks.

You need to revisit the place that use the isPredicable bit of the MI, which might cause functionality change. i.e.

bool TargetInstrInfo::isUnpredicatedTerminator(const MachineInstr &MI) const {
  if (!MI.isTerminator()) return false;

  // Conditional branch is a special case.
  if (MI.isBranch() && !MI.isBarrier())
    return true;
  if (!MI.isPredicable())
    return true;
  return !isPredicated(MI);
}

bool TargetInstrInfo::PredicateInstruction(
    MachineInstr &MI, ArrayRef<MachineOperand> Pred) const {
  bool MadeChange = false;

  assert(!MI.isBundle() &&
         "TargetInstrInfo::PredicateInstruction() can't handle bundles");

  const MCInstrDesc &MCID = MI.getDesc();
  if (!MI.isPredicable())
    return false;

  for (unsigned j = 0, i = 0, e = MI.getNumOperands(); i != e; ++i) {
    if (MCID.OpInfo[i].isPredicate()) {
      MachineOperand &MO = MI.getOperand(i);
      if (MO.isReg()) {
        MO.setReg(Pred[j].getReg());
        MadeChange = true;
      } else if (MO.isImm()) {
        MO.setImm(Pred[j].getImm());
        MadeChange = true;
      } else if (MO.isMBB()) {
        MO.setMBB(Pred[j].getMBB());
        MadeChange = true;
      }
      ++j;
    }
  }
  return MadeChange;
}
qiucf added a comment.Dec 29 2019, 7:30 PM

You need to revisit the place that use the isPredicable bit of the MI, which might cause functionality change.

If this is a NFC change, please add [NFC] in summary. Or else, please add test. Thanks.

I checked all references to isPredicable in codebase:

  • Instr doc generation uses the bit (this is non-functional)
  • CodeGen tablegen itself uses it
  • Both TargetInstrInfo::isUnpredicatedTerminator and TargetInstrInfo::PredicateInstruction references the method, but PPC overrides them and doesn't use it.
  • MachineInstr::findFirstPredOperandIdx references it, but only ARM and AMDGPU invokes it
  • Implicit null check and machine sink have referenced it. However, this patch won't touch instrs may load or store, so result of expression would never change
  • Other references are for other platforms
steven.zhang accepted this revision.Dec 29 2019, 10:22 PM

LGTM as it makes sense. But please hold on for some days in case someone else have other concern. And I suggest you commit follow up patches to set this bit for other branch instructions.

This revision is now accepted and ready to land.Dec 29 2019, 10:22 PM
jsji added a comment.Dec 30 2019, 6:50 PM

I checked all references to isPredicable in codebase:

  • Instr doc generation uses the bit (this is non-functional)
  • CodeGen tablegen itself uses it
  • Both TargetInstrInfo::isUnpredicatedTerminator and TargetInstrInfo::PredicateInstruction references the method, but PPC overrides them and doesn't use it.
  • MachineInstr::findFirstPredOperandIdx references it, but only ARM and AMDGPU invokes it
  • Implicit null check and machine sink have referenced it. However, this patch won't touch instrs may load or store, so result of expression would never change
  • Other references are for other platforms

So, I assume you are saying this is *NFC* for PowerPC, right?

qiucf retitled this revision from [PowerPC] Use isPredicable bits in instruction definitions to [NFC] [PowerPC] Use isPredicable bits in instruction definitions.Dec 30 2019, 7:12 PM

I checked all references to isPredicable in codebase:

  • Instr doc generation uses the bit (this is non-functional)
  • CodeGen tablegen itself uses it
  • Both TargetInstrInfo::isUnpredicatedTerminator and TargetInstrInfo::PredicateInstruction references the method, but PPC overrides them and doesn't use it.
  • MachineInstr::findFirstPredOperandIdx references it, but only ARM and AMDGPU invokes it
  • Implicit null check and machine sink have referenced it. However, this patch won't touch instrs may load or store, so result of expression would never change
  • Other references are for other platforms

So, I assume you are saying this is *NFC* for PowerPC, right?

Yes. I changed title of this revision.

This revision was automatically updated to reflect the committed changes.