[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 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.


================
Comment at: lib/CodeGen/TailDuplication.cpp:752
@@ -754,1 +751,3 @@
 
+    if (PredTBB)
+      TII->InsertBranch(*PredBB, PredTBB, PredFBB, PredCond, DebugLoc());
----------------
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.

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


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3930
@@ +3929,3 @@
+    }
+    assert(!FallthroughBB && "Found more than one fallthrough successor.");
+    FallthroughBB = *SI;
----------------
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.


http://reviews.llvm.org/D11393