<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 20, 2015 at 4:41 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the cleanup. Responses inline to some parts.<div><div class="h5"><br>
On 02/20/2015 03:44 PM, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: dblaikie<br>
Date: Fri Feb 20 17:44:24 2015<br>
New Revision: 230094<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=230094&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=230094&view=rev</a><br>
Log:<br>
Remove some unnecessary unreachables in favor of (sometimes implicit) assertions<br>
<br>
Also simplify some else-after-return cases including some standard<br>
algorithm convenience/use.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/IR/<u></u>Instructions.h<br>
llvm/trunk/lib/Transforms/<u></u>Scalar/<u></u>RewriteStatepointsForGC.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/IR/<u></u>Instructions.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=230094&r1=230093&r2=230094&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/include/<u></u>llvm/IR/Instructions.h?rev=<u></u>230094&r1=230093&r2=230094&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/include/llvm/IR/<u></u>Instructions.h (original)<br>
+++ llvm/trunk/include/llvm/IR/<u></u>Instructions.h Fri Feb 20 17:44:24 2015<br>
@@ -2166,6 +2166,8 @@ public:<br>
return block_begin() + getNumOperands();<br>
}<br>
+ op_range incoming_values() { return operands(); }<br>
+<br>
/// getNumIncomingValues - Return the number of incoming edges<br>
///<br>
unsigned getNumIncomingValues() const { return getNumOperands(); }<br>
<br>
Modified: llvm/trunk/lib/Transforms/<u></u>Scalar/<u></u>RewriteStatepointsForGC.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230094&r1=230093&r2=230094&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>Transforms/Scalar/<u></u>RewriteStatepointsForGC.cpp?<u></u>rev=230094&r1=230093&r2=<u></u>230094&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Transforms/<u></u>Scalar/<u></u>RewriteStatepointsForGC.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/<u></u>Scalar/<u></u>RewriteStatepointsForGC.cpp Fri Feb 20 17:44:24 2015<br>
@@ -150,17 +150,16 @@ static bool isLiveGCReferenceAt(Value &V<br>
static bool isAggWhichContainsGCPtrType(<u></u>Type *Ty) {<br>
if (VectorType *VT = dyn_cast<VectorType>(Ty))<br>
return isGCPointerType(VT-><u></u>getScalarType());<br>
- else if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {<br>
+ if (ArrayType *AT = dyn_cast<ArrayType>(Ty))<br>
return isGCPointerType(AT-><u></u>getElementType()) ||<br>
isAggWhichContainsGCPtrType(<u></u>AT->getElementType());<br>
- } else if (StructType *ST = dyn_cast<StructType>(Ty)) {<br>
- bool UnsupportedType = false;<br>
- for (Type *SubType : ST->subtypes())<br>
- UnsupportedType |=<br>
- isGCPointerType(SubType) || isAggWhichContainsGCPtrType(<u></u>SubType);<br>
- return UnsupportedType;<br>
- } else<br>
- return false;<br>
+ if (StructType *ST = dyn_cast<StructType>(Ty))<br>
+ return std::any_of(ST->subtypes().<u></u>begin(), ST->subtypes().end(),<br>
+ [](Type *SubType) {<br>
+ return isGCPointerType(SubType) ||<br>
+ isAggWhichContainsGCPtrType(<u></u>SubType);<br>
+ });<br>
+ return false;<br>
</blockquote></div></div>
I feel the resulting code is less readable. I realize that these are return statements inside the conditions, but I'd still prefer the other pattern. This is effectively as close to a match clause as C++ can get. I also release I'm in conflict of the letter of the coding standard. However, this is a case where I believe the coding standard is wrong because it's producing less readable code.</blockquote><div><br>What is it about this code do you think is a readability hit compared to other instances of this style guide requirement?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
#endif<br>
@@ -379,12 +378,9 @@ static Value *findBaseDefiningValue(Valu<br>
Value *def = CI->stripPointerCasts();<br>
assert(def->getType()-><u></u>isPointerTy() &&<br>
"Base for pointer must be another pointer");<br>
- if (isa<CastInst>(def)) {<br>
- // If we find a cast instruction here, it means we've found a cast<br>
- // which is not simply a pointer cast (i.e. an inttoptr). We don't<br>
- // know how to handle int->ptr conversion.<br>
- llvm_unreachable("Can not find the base pointers for an inttoptr cast");<br>
- }<br>
+ // If we find a cast instruction here, it means we've found a cast which is<br>
+ // not simply a pointer cast (i.e. an inttoptr). We don't know how to<br>
+ // handle int->ptr conversion.<br>
assert(!isa<CastInst>(def) && "shouldn't find another cast here");<br>
return findBaseDefiningValue(def);<br>
}<br>
@@ -492,13 +488,8 @@ static Value *findBaseDefiningValue(Valu<br>
if (SelectInst *select = dyn_cast<SelectInst>(I)) {<br>
return select;<br>
}<br>
- if (PHINode *phi = dyn_cast<PHINode>(I)) {<br>
- return phi;<br>
- }<br>
- errs() << "unknown type: " << *I << "\n";<br>
- llvm_unreachable("unknown type");<br>
- return nullptr;<br>
+ return cast<PHINode>(I);<br>
</blockquote></div></div>
The llvm_unreachable should probably become a report_fatal_error w/a decent error message, but your version looses information that is useful in debugging this information. It's fully possible that I didn't handle all instruction types here. Having a clean failure is important.</blockquote><div><br>Usually a comment preceeding the cast should suffice to provide context (the cast will fail, give a generic assertion, you'll go up to where the cast is & find a comment explaining why that cast should be true) - but a separate assert(isa<PHINode>()) with a message could be used if desired. Usually we remove those in favor of just having the cast fail, though.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
/// Returns the base defining value for this value.<br>
@@ -635,18 +626,18 @@ private:<br>
case PhiState::Base:<br>
assert(stateA.getBase() && "can't be null");<br>
- if (stateB.isUnknown()) {<br>
+ if (stateB.isUnknown())<br>
return stateA;<br>
- } else if (stateB.isBase()) {<br>
+<br>
+ if (stateB.isBase()) {<br>
if (stateA.getBase() == stateB.getBase()) {<br>
assert(stateA == stateB && "equality broken!");<br>
return stateA;<br>
}<br>
return PhiState(PhiState::Conflict);<br>
- } else {<br>
- assert(stateB.isConflict() && "only three states!");<br>
- return PhiState(PhiState::Conflict);<br>
}<br>
+ assert(stateB.isConflict() && "only three states!");<br>
+ return PhiState(PhiState::Conflict);<br>
case PhiState::Conflict:<br>
return stateA;<br>
@@ -701,10 +692,9 @@ static Value *findBasePointer(Value *I,<br>
Value *v = Pair.first;<br>
assert(!isKnownBaseResult(v) && "why did it get added?");<br>
if (PHINode *phi = dyn_cast<PHINode>(v)) {<br>
- unsigned NumPHIValues = phi->getNumIncomingValues();<br>
- assert(NumPHIValues > 0 && "zero input phis are illegal");<br>
- for (unsigned i = 0; i != NumPHIValues; ++i) {<br>
- Value *InVal = phi->getIncomingValue(i);<br>
+ assert(phi-><u></u>getNumIncomingValues() > 0 &&<br>
+ "zero input phis are illegal");<br>
+ for (Value *InVal : phi->incoming_values()) {<br>
Value *local = findBaseOrBDV(InVal, cache);<br>
if (!isKnownBaseResult(local) && states.find(local) == states.end()) {<br>
states[local] = PhiState();<br>
@@ -748,18 +738,12 @@ static Value *findBasePointer(Value *I,<br>
MeetPhiStates calculateMeet(states);<br>
Value *v = Pair.first;<br>
assert(!isKnownBaseResult(v) && "why did it get added?");<br>
- assert(isa<SelectInst>(v) || isa<PHINode>(v));<br>
if (SelectInst *select = dyn_cast<SelectInst>(v)) {<br>
calculateMeet.meetWith(<u></u>findBaseOrBDV(select-><u></u>getTrueValue(), cache));<br>
calculateMeet.meetWith(<u></u>findBaseOrBDV(select-><u></u>getFalseValue(), cache));<br>
- } else if (PHINode *phi = dyn_cast<PHINode>(v)) {<br>
- for (unsigned i = 0; i < phi->getNumIncomingValues(); i++) {<br>
- calculateMeet.meetWith(<br>
- findBaseOrBDV(phi-><u></u>getIncomingValue(i), cache));<br>
- }<br>
- } else {<br>
- llvm_unreachable("no such state expected");<br>
- }<br>
+ } else<br>
+ for (Value *Val : cast<PHINode>(v)->incoming_<u></u>values())<br>
+ calculateMeet.meetWith(<u></u>findBaseOrBDV(Val, cache));<br>
PhiState oldState = states[v];<br>
PhiState newState = calculateMeet.getResult();<br>
@@ -989,13 +973,13 @@ static void findBasePointers(const State<br>
cast<Instruction>(ptr)-><u></u>getParent())) &&<br>
"The base we found better dominate the derived pointer");<br>
- if (isNullConstant(base))<br>
- // If you see this trip and like to live really dangerously, the code<br>
- // should be correct, just with idioms the verifier can't handle. You<br>
- // can try disabling the verifier at your own substaintial risk.<br>
- llvm_unreachable("the relocation code needs adjustment to handle the"<br>
- "relocation of a null pointer constant without causing"<br>
- "false positives in the safepoint ir verifier.");<br>
+ // If you see this trip and like to live really dangerously, the code should<br>
+ // be correct, just with idioms the verifier can't handle. You can try<br>
+ // disabling the verifier at your own substaintial risk.<br>
+ assert(!isNullConstant(base) && "the relocation code needs adjustment to "<br>
+ "handle the relocation of a null pointer "<br>
+ "constant without causing false positives "<br>
+ "in the safepoint ir verifier.");<br>
}<br>
}<br>
@@ -1258,7 +1242,7 @@ makeStatepointExplicitImpl(<u></u>const CallSit<br>
Builder.SetInsertPoint(IP);<br>
Builder.<u></u>SetCurrentDebugLocation(IP-><u></u>getDebugLoc());<br>
- } else if (CS.isInvoke()) {<br>
+ } else {<br>
InvokeInst *toReplace = cast<InvokeInst>(CS.<u></u>getInstruction());<br>
// Insert the new invoke into the old block. We'll remove the old one in a<br>
@@ -1309,8 +1293,6 @@ makeStatepointExplicitImpl(<u></u>const CallSit<br>
// gc relocates will be generated later as if it were regular call<br>
// statepoint<br>
- } else {<br>
- llvm_unreachable("unexpect type of CallSite");<br>
}<br>
assert(token);<br>
@@ -1526,12 +1508,11 @@ static void relocationViaAlloca(<br>
if (auto II = dyn_cast<InvokeInst>(<u></u>Statepoint)) {<br>
InsertClobbersAt(II-><u></u>getNormalDest()-><u></u>getFirstInsertionPt());<br>
InsertClobbersAt(II-><u></u>getUnwindDest()-><u></u>getFirstInsertionPt());<br>
- } else if (auto CI = dyn_cast<CallInst>(Statepoint)<u></u>) {<br>
- BasicBlock::iterator Next(CI);<br>
+ } else {<br>
+ BasicBlock::iterator Next(cast<CallInst>(<u></u>Statepoint));<br>
Next++;<br>
InsertClobbersAt(Next);<br>
- } else<br>
- llvm_unreachable("illegal statepoint instruction type?");<br>
+ }<br>
#endif<br>
}<br>
// update use with load allocas and add store for gc_relocated<br>
<br>
<br>
______________________________<u></u>_________________<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/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
</div></div></blockquote></div><br></div></div>