[PATCH] D21726: CodeGen: Use MachineInstr& in TargetInstrInfo, NFC

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Jun-25, at 09:42, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 
> ab added a subscriber: ab.
> ab accepted this revision.
> ab added a reviewer: ab.
> ab added a comment.
> This revision is now accepted and ready to land.
> 
> Everything LGTM, but you might want to wait for a third pair of eyes.  A few inline comments.
> 
> Also, I see BPF doesn't need any changes; can you confirm?
> 
> 
> ================
> Comment at: include/llvm/Target/TargetInstrInfo.h:1197
> @@ +1196,3 @@
> +  /// Compute and return the latency of the given data dependent def and use
> +  /// when the operand indices are already known. UseMI may be NULL for an
> +  /// unknown use.
> ----------------
> Might as well fix NULL -> null/nullptr

Done.

> 
> ================
> Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:838
> @@ -840,1 +837,3 @@
> +    const TargetRegisterInfo *TRI, const AccessKind AccessToCheck = AK_All) {
> // We iterate backward starting \p To until we hit \p From
> +  MachineBasicBlock::iterator B = To->getParent()->begin();
> ----------------
> Move this comment to the for loop below?

Done.

> ================
> Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:839
> @@ -840,3 +838,3 @@
> // We iterate backward starting \p To until we hit \p From
> -  MachineBasicBlock::iterator I = To, E = From, B = To->getParent()->begin();
> +  MachineBasicBlock::iterator B = To->getParent()->begin();
> 
> ----------------
> and remove B and do
> 
>  if (To == To->getParent()->begin())
> 
> below?

Done.

> ================
> Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1132
> @@ -1134,3 +1131,3 @@
> MI->setDesc(get(NewOpc));
> -  CmpInstr->eraseFromParent();
> -  bool succeeded = UpdateOperandRegClass(MI);
> +  CmpInstr.eraseFromParent();
> +  bool succeeded = UpdateOperandRegClass(*MI);
> ----------------
> Here and elsewhere: It's not new and unlikely to become a problem, but I'm a tad worried about eraseFromParent() on references (though we usually do nothing to prevent reusing erased pointers either, at least we could, I guess).  WDYT?
> 
> ================
> Comment at: lib/Target/AMDGPU/R600ExpandSpecialInstrs.cpp:116
> @@ -115,3 +115,3 @@
>       }
>       MI.eraseFromParent();
>       continue;
> ----------------
> Heh, I guess we already do that

Yup, it's all over.  But I also don't really mind it, since ASan should catch the bugs.

> 
> 
> http://reviews.llvm.org/D21726
> 
> 
>