[PATCH] D20017: Aggressive choosing best loop top

[Chandler Carruth via llvm-commits]

chandlerc added a reviewer: davidxl.
chandlerc added a comment.

Have you benchmarked how this changes performance on other architectures? If you can't we'll need to get others do to so, as this is likely to have pretty widespread effects.

Also should get some of the other folks who've bene looking at loop layout to look at this. CC'ing David at least.


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:900
@@ -898,2 +899,3 @@
   BasicBlock *LoopBB = BasicBlock::Create(Ctx, "atomicrmw.start", F, ExitBB);
+  MDNode *BrWeight = MDBuilder(AI->getContext()).createBranchWeights(1, 1);
 
----------------
Sink this to where it is used?

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:932-934
@@ -929,3 +931,5 @@
 
-  Builder.CreateCondBr(Success, ExitBB, LoopBB);
+  // Set LoopBB -> ExitBB and LoopBB -> LoopBB with 50% probability, so block
+  // placement won't apply aggressive-best-top on LoopBB
+  Builder.CreateCondBr(Success, ExitBB, LoopBB, BrWeight);
 
----------------
This comment doesn't really help here. What is "aggressive-best-top" and what does it mean?

I think what you're actually doing is trying to give probabilities to override the default loop probabilities because the spin loop for an atomic isn't expected to loop often if at all. I'm actually surprised you would have a 50% probability of looping here, I would have expected a relatively low probability of spinning to be more appropriate.

I think you should:

1) split this out into an independent change with its own description and test case
2) improve this comment to talk generically about the fact that the loop isn't expected to be taken unless there is contention.

Then the rest of this change can be predicated on that. I'm assuming that you can test this independently because its actually annotating the IR.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:114-117
@@ -113,1 +113,6 @@
 
+static cl::opt<bool>
+    AggressiveBestTop("aggressive-best-top",
+    cl::desc("Find best top from all latches even with conditional exit."),
+    cl::init(true), cl::Hidden);
+
----------------
Any particular reason for a flag? Or for calling this aggressive? It seems pretty straight forward to try to select the best loop top among latches.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:780
@@ +779,3 @@
+      // Don't handle if loop.header -> latch is very cold:
+      // e.g. SystemZ atomicrmw instruction (atomicrmw-minmax-*.ll)
+      //
----------------
I would't reference system-z or a test case which might go away. You have the generic description here and can add details about why this is bad in a generic sense.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:789
@@ +788,3 @@
+      //
+      const BranchProbability VeryColdProb(1, 1000); // 99.9%
+      if (L.getHeader()->isSuccessor(Pred) &&
----------------
Why would you handle it if cold at all? Not clear why you need a different threshold here than the threshold used throughout this code for cold.

================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:5298-5299
@@ -5295,3 +5297,4 @@
     .addImm(SystemZ::CCMASK_ICMP).addImm(KeepOldMask).addMBB(UpdateMBB);
-  MBB->addSuccessor(UpdateMBB);
-  MBB->addSuccessor(UseAltMBB);
+  // Set LoopMBB -> UpdateMBB with VeryLowProb, so Block Placement Pass will
+  // layout in this order: LoopMBB UseAltMBB UpdateMBB
+  MBB->addSuccessor(UpdateMBB, VeryLowProb);
----------------
Much like in the atomic expand pass, I'd talk about the semantic *reason* why these probabilities are correct, rather than about some hypothetical layout.

Is this testable independently? If so, I would separate this too into its own patch.


http://reviews.llvm.org/D20017