[PATCH] D34769: [X86] X86::CMOV to Branch heuristic based optimization

[Amjad Aboud via Phabricator via llvm-commits]

aaboud marked 2 inline comments as done.
aaboud added a comment.

Thanks Zvi,
I answered your comments below.



================
Comment at: lib/Target/X86/X86CmovConversion.cpp:338
+        bool IsCMOV = CmovInstructions.count(&MI);
+        for (auto &MO : MI.operands()) {
+          if (!MO.isReg() || !MO.isUse())
----------------
zvi wrote:
> Are these two equivalent?
> 
>   for (auto &MO : MI.operands())
>     if (!MO.isReg() || !MO.isUse())
>       continue;
> 
> and
> 
>   for (auto &MO : MI.uses())
>     if (!MO.isReg())
>       continue?
Could be, but to keep the code consistent wit the below loop, I preferred this format, I do not mind change to your suggestion if you think it is better.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:343
+          auto &RDM = RegDefMaps[TargetRegisterInfo::isVirtualRegister(Reg)];
+          MachineInstr *DefMI = RDM.count(Reg) ? RDM[Reg] : nullptr;
+          OperandToDefMap[&MO] = DefMI;
----------------
zvi wrote:
> Please avoid using operator[] for lookups - use DenseMap::lookup() instead.
> And maybe mapping operands to null's is just inefficient? The non-existence of the mapping can be also used for choosing depth = 0?
> So this code can be instead something like:
> 
>   if (MachineInstr *DefMI = RDM.lookup(Reg)) {
>     OperandToDefMap[&MO] = DefMI;
>     MIDepth = std::max(MIDepth, DepthMap.lookup(DefMI).Depth);
>     if (!IsCMOV)
>       MIDepthOpt = std::max(MIDepthOpt, DepthMap.lookup(DefMI).OptDepth);
>    }
> 
> This also applies to more uses of operator[] below. If I'm not mistaken, only these two statements should remain using opertor[]:
> 
>   RegDefMaps[TargetRegisterInfo::isVirtualRegister(Reg)][Reg] = &MI;
>   DepthMap[&MI] = {MIDepth += Latency, MIDepthOpt += Latency};
> 
> 
Fixed most places.
Other places where I kept the "[]" operator, we know for sure that the entry we are looking for exists in the container.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:355
+
+        for (auto &MO : MI.operands()) {
+          if (!MO.isReg() || !MO.isDef())
----------------
zvi wrote:
> Are these two equivalent?
> 
>   for (auto &MO: MI.operands())
>     if (!MO.isReg() || !MO.isDef())
>       continue;
> 
> and
> 
>   for (auto &MO: MI.defs())
>    
No they are not, the "defs()" does not include implicit definition operands.
```
  iterator_range<mop_iterator> operands() {
    return make_range(operands_begin(), operands_end());
  }

  /// Returns a range over all explicit operands that are register definitions.
  /// Implicit definition are not included!
  iterator_range<mop_iterator> defs() {
    return make_range(operands_begin(),
                      operands_begin() + getDesc().getNumDefs());
  }

  /// Returns a range that includes all operands that are register uses.
  /// This may include unrelated operands which are not register uses.
  iterator_range<mop_iterator> uses() {
    return make_range(operands_begin() + getDesc().getNumDefs(),
                      operands_end());
  }
```


https://reviews.llvm.org/D34769