[PATCH] D66399: [ORCv2] - New Speculate Query Implementation

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

LGTM, pending else-after-return fixes.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Speculation.h:85-88
+      if (It != GlobalSpecMap.end())
         CandidateSet = It->getSecond();
+      else
+        return;
----------------
This is not quite right.

"else after return" refers to the following idiom:

  if (C) {
    // do stuff
    return;
  } else {
    // do other stuff.
  }

LLVM's style guide recommends that this be rewritten as:

  if (C) {
    // do stuff
    return;
  }
  // do other stuff.

In this case the right fix was to replace:

  if (It == GlobalSpecMap.end() || Iv.second == false)
    return;
  else
    CandidateSet = It->getSecond();

with

  if (It == GlobalSpecMap.end() || Iv.second == false)
    return;

  CandidateSet = It->getSecond();


================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:130-134
+  if (TotalBlocks == 1)
+    return TotalBlocks;
+  else
+    return TotalBlocks / 2;
+}
----------------
To remove the else-after-return here, this should be rewritten from:

  if (TotalBlocks == 1)
    return TotalBlocks;
  else
    return TotalBlocks / 2;

to:

  if (TotalBlocks == 1)
    return TotalBlocks;
  return TotalBlocks / 2;



================
Comment at: llvm/lib/ExecutionEngine/Orc/SpeculateAnalyses.cpp:201-204
+    if (Itr->second.Downward)
+      Itr->second.Downward = false;
+    else
+      return;
----------------
This can be rewritten as:

  if (!Itr->second.Downward)
    return;
  Itr->second.Downward = false;


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66399/new/

https://reviews.llvm.org/D66399