<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 20, 2015 at 2:45 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div>
<br>
<div>On 02/20/2015 02:24 PM, David Blaikie
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Feb 20, 2015 at 2:05 PM,
Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author:
reames<br>
Date: Fri Feb 20 16:05:18 2015<br>
New Revision: 230068<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=230068&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=230068&view=rev</a><br>
Log:<br>
[RewriteStatepointsForGC] More style cleanup [NFC]<br>
<br>
Use llvm_unreachable where appropriate, use SmallVector
where easy to do so, introduce typedefs for planned type
migrations.<br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp<br>
<br>
Modified:
llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230068&r1=230067&r2=230068&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230068&r1=230067&r2=230068&view=diff</a><br>
==============================================================================<br>
---
llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
(original)<br>
+++
llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
Fri Feb 20 16:05:18 2015<br>
@@ -96,10 +96,11 @@ namespace {<br>
// base relation will remain. Internally, we add a
mixture of the two<br>
// types, then update all the second type to the first
type<br>
typedef std::map<Value *, Value *>
DefiningValueMapTy;<br>
+typedef std::set<llvm::Value *>
StatepointLiveSetTy;<br>
<br>
struct PartiallyConstructedSafepointRecord {<br>
/// The set of values known to be live accross this
safepoint<br>
- std::set<llvm::Value *> liveset;<br>
+ StatepointLiveSetTy liveset;<br>
<br>
/// Mapping from live pointers to a base-defining-value<br>
DenseMap<llvm::Value *, llvm::Value *>
PointerToBase;<br>
@@ -283,7 +284,7 @@
analyzeParsePointLiveness(DominatorTree<br>
// Note: This output is used by several of the test
cases<br>
// The order of elemtns in a set is not stable, put
them in a vec and sort<br>
// by name<br>
- std::vector<Value *> temp;<br>
+ SmallVector<Value *, 64> temp;<br>
temp.insert(temp.end(), liveset.begin(),
liveset.end());<br>
std::sort(temp.begin(), temp.end(), order_by_name);<br>
errs() << "Live Variables:\n";<br>
@@ -583,13 +584,14 @@ private:<br>
Value *base; // non null only if status == base<br>
};<br>
<br>
+typedef std::map<Value *, PhiState>
ConflictStateMapTy;<br>
// Values of type PhiState form a lattice, and this is a
helper<br>
// class that implementes the meet operation. The meat
of the meet<br>
// operation is implemented in MeetPhiStates::pureMeet<br>
class MeetPhiStates {<br>
public:<br>
// phiStates is a mapping from PHINodes and
SelectInst's to PhiStates.<br>
- explicit MeetPhiStates(const std::map<Value *,
PhiState> &phiStates)<br>
+ explicit MeetPhiStates(const ConflictStateMapTy
&phiStates)<br>
: phiStates(phiStates) {}<br>
<br>
// Destructively meet the current result with the base
V. V can<br>
@@ -607,7 +609,7 @@ public:<br>
PhiState getResult() const { return currentResult; }<br>
<br>
private:<br>
- const std::map<Value *, PhiState> &phiStates;<br>
+ const ConflictStateMapTy &phiStates;<br>
PhiState currentResult;<br>
<br>
/// Return a phi state for a base defining value.
We'll generate a new<br>
@@ -687,7 +689,7 @@ static Value *findBasePointer(Value
*I,<br>
// analougous to pessimistic data flow and would likely
lead to an<br>
// overall worse solution.<br>
<br>
- std::map<Value *, PhiState> states;<br>
+ ConflictStateMapTy states;<br>
states[def] = PhiState();<br>
// Recursively fill in all phis & selects reachable
from the initial one<br>
// for which we don't already know a definite base
value for<br>
@@ -820,9 +822,8 @@ static Value *findBasePointer(Value
*I,<br>
v->getParent()->getParent()->getParent()->getContext(),
MDConst);<br>
basesel->setMetadata("is_base_value", md);<br>
states[v] = PhiState(PhiState::Conflict,
basesel);<br>
- } else {<br>
- assert(false);<br>
- }<br>
+ } else<br>
+ llvm_unreachable("unknown conflict type");<br>
</blockquote>
<div><br>
Rather than llvm_unreachable in an else - take the 'if'
and move it into an assert instead?<br>
</div>
</div>
</div>
</div>
</blockquote></div></div>
This is an if-elseif-else chain. I could check both the if and
else-if conditions in an assert before hand, but that seems less
readable. This could also be arranged as a switch, but then I'd
loss the dyn_cast. </div></blockquote><div><br>I was thinking of something simpler - changing the if condition to an assert inside the else block:<br><br>if (x)<br> ...<br>else if (y)<br> ...<br>else<br> unreachable<br><br>-><br><br>if (x)<br> ...<br>else<br> assert (y)<br> ...<br><br>In some cases, dyn_cast just becomes a cast and there's no need for the assert at all (teh cast will assert) - sometimes no need for a named variable if it's only used once.<br><br>I've committed a bunch of cleanup of unreachables in this file, including this one, in <span style="color:rgb(0,0,0)">r230094</span><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"> I'm open to any of these, but given there's no
good choice, I'd probably stick with what's here unless you have a
strong preference. <br><div><div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
}<br>
}<br>
<br>
@@ -922,9 +923,8 @@ static Value *findBasePointer(Value
*I,<br>
}<br>
basesel->setOperand(i, base);<br>
}<br>
- } else {<br>
- assert(false && "unexpected type");<br>
- }<br>
+ } else<br>
+ llvm_unreachable("unexpected conflict type");<br>
}<br>
}<br>
<br>
@@ -1356,7 +1356,7 @@ static void
stablize_order(SmallVectorIm<br>
SmallVectorImpl<Value *>
&livevec) {<br>
assert(basevec.size() == livevec.size());<br>
<br>
- std::vector<name_ordering> temp;<br>
+ SmallVector<name_ordering, 64> temp;<br>
for (size_t i = 0; i < basevec.size(); i++) {<br>
name_ordering v;<br>
v.base = basevec[i];<br>
@@ -1654,9 +1654,8 @@ static void
insertUseHolderAfter(CallSit<br>
Func, Values, "",
invoke->getUnwindDest()->getFirstInsertionPt());<br>
holders.push_back(normal_holder);<br>
holders.push_back(unwind_holder);<br>
- } else {<br>
- assert(false && "Unsupported");<br>
- }<br>
+ } else<br>
+ llvm_unreachable("unsupported call type");<br>
}<br>
<br>
static void findLiveReferences(<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>