[PATCH] D11393: [X86] Allow X86::COND_NE_OR_P and X86::COND_NP_OR_E to be reversed.

[David Kreitzer via llvm-commits]

DavidKreitzer added a comment.

Thanks for following up on this. I just have a couple minor additional commenting suggestions.

To be clear, I was only suggesting that you get another reviewer for the change in include/llvm/Target/TargetInstrInfo.h. I would like someone to confirm that we can reasonably expect the MBB CFG information to be valid when AnalyzeBranch and InsertBranch are called. Aside from that, I am comfortable approving the rest of the patch myself. As for who should review the TargetInstrInfo.h change, maybe Sanjay or Nadav can do that? Also, CODE_OWNERS.TXT lists Evan Cheng as the CodeGen owner, though I don't know how up-to-date that is.

Thanks!
-Dave


================
Comment at: include/llvm/Target/TargetInstrInfo.h:455
@@ -454,2 +454,3 @@
   ///
+  /// The CFG information must be valid before calling this function.
   virtual bool AnalyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
----------------
Both here and at 526, I would recommend saying explicitly, "The CFG information in MBB.Predecessors and MBB.Successors must be valid before calling this function."

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3920
@@ +3919,3 @@
+// Given a MBB and its TBB, find the FBB which was a fallthrough MBB (it may not
+// be a fallthorough MBB now due to layout changes).
+static MachineBasicBlock *getFallThroughMBB(MachineBasicBlock *MBB,
----------------
Maybe add another sentence to this comment: "Return nullptr if the fallthough MBB cannot be identified."



http://reviews.llvm.org/D11393