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

[Cong Hou via llvm-commits]

congh added a comment.

In http://reviews.llvm.org/D11393#341155, @DavidKreitzer wrote:

> Thanks for the fixes! Just a few more minor issues.
>
> Regarding my suggestion to modify the comments for InsertBranch/AnalyzeBranch: I would recommend that after doing so you run the change by the current CodeGen code owner for approval, since you are adding a dependence that doesn't currently exist, namely that these routines expect the CFG links in MachineBasicBlock to be up-to-date.


I am really sorry for replying your comments so late (I was in vacation in Feb). This sounds good to me. Whom do you recommend as the CodeGen code owner to review this patch? Thanks!


================
Comment at: lib/CodeGen/TailDuplication.cpp:752
@@ -754,1 +751,3 @@
 
+    if (PredTBB)
+      TII->InsertBranch(*PredBB, PredTBB, PredFBB, PredCond, DebugLoc());
----------------
DavidKreitzer wrote:
> So, I understand why you needed to make this change. But it suggests that you might want to update the comment for InsertBranch in TargetInstrInfo.h to say that the CFG information must be valid before calling the routine. Similarly for AnalyzeBranch.
OK, I have added the comments suggested by you to those routines.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3925
@@ +3924,3 @@
+      continue;
+    if (FallthroughBB) {
+      FallthroughBB->dump();
----------------
DavidKreitzer wrote:
> Did you intentionally leave this debugging code here?
> 
No.. my bad. I have removed them.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3930
@@ +3929,3 @@
+    }
+    assert(!FallthroughBB && "Found more than one fallthrough successor.");
+    FallthroughBB = *SI;
----------------
DavidKreitzer wrote:
> This assertion seems dangerous to me given that you are calling this routine from within AnalyzeBranch. Theoretically, you could be in the middle of analyzing an unsupported block like this:
> 
> JA     B1
> JNP   B2
> JNE   B3
> .... fallthrough to B2 ...
> 
> That would trigger a call to getFallThroughMBB with TBB == B3, and this assertion would fail when it sees the two other successors B1 & B2. I don't know whether it is even possible to get IR like this, but it seems like this code ought to tolerate it.
> 
> I'd recommend simply returning nullptr if you find multiple non-EH, non-TBB successors.
OK. This makes sense.


http://reviews.llvm.org/D11393