[llvm] r230094 - Remove some unnecessary unreachables in favor of (sometimes implicit) assertions
David Blaikie
dblaikie at gmail.com
Fri Feb 20 17:00:06 PST 2015
On Fri, Feb 20, 2015 at 4:41 PM, Philip Reames <listmail at philipreames.com>
wrote:
> Thanks for the cleanup. Responses inline to some parts.
>
> On 02/20/2015 03:44 PM, David Blaikie wrote:
>
>> Author: dblaikie
>> Date: Fri Feb 20 17:44:24 2015
>> New Revision: 230094
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=230094&view=rev
>> Log:
>> Remove some unnecessary unreachables in favor of (sometimes implicit)
>> assertions
>>
>> Also simplify some else-after-return cases including some standard
>> algorithm convenience/use.
>>
>> Modified:
>> llvm/trunk/include/llvm/IR/Instructions.h
>> llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>>
>> Modified: llvm/trunk/include/llvm/IR/Instructions.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
>> llvm/IR/Instructions.h?rev=230094&r1=230093&r2=230094&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/include/llvm/IR/Instructions.h (original)
>> +++ llvm/trunk/include/llvm/IR/Instructions.h Fri Feb 20 17:44:24 2015
>> @@ -2166,6 +2166,8 @@ public:
>> return block_begin() + getNumOperands();
>> }
>> + op_range incoming_values() { return operands(); }
>> +
>> /// getNumIncomingValues - Return the number of incoming edges
>> ///
>> unsigned getNumIncomingValues() const { return getNumOperands(); }
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230094&r1=230093&r2=
>> 230094&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>> (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp Fri Feb
>> 20 17:44:24 2015
>> @@ -150,17 +150,16 @@ static bool isLiveGCReferenceAt(Value &V
>> static bool isAggWhichContainsGCPtrType(Type *Ty) {
>> if (VectorType *VT = dyn_cast<VectorType>(Ty))
>> return isGCPointerType(VT->getScalarType());
>> - else if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {
>> + if (ArrayType *AT = dyn_cast<ArrayType>(Ty))
>> return isGCPointerType(AT->getElementType()) ||
>> isAggWhichContainsGCPtrType(AT->getElementType());
>> - } else if (StructType *ST = dyn_cast<StructType>(Ty)) {
>> - bool UnsupportedType = false;
>> - for (Type *SubType : ST->subtypes())
>> - UnsupportedType |=
>> - isGCPointerType(SubType) || isAggWhichContainsGCPtrType(
>> SubType);
>> - return UnsupportedType;
>> - } else
>> - return false;
>> + if (StructType *ST = dyn_cast<StructType>(Ty))
>> + return std::any_of(ST->subtypes().begin(), ST->subtypes().end(),
>> + [](Type *SubType) {
>> + return isGCPointerType(SubType) ||
>> + isAggWhichContainsGCPtrType(SubType);
>> + });
>> + return false;
>>
> 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.
What is it about this code do you think is a readability hit compared to
other instances of this style guide requirement?
>
> }
>> #endif
>> @@ -379,12 +378,9 @@ static Value *findBaseDefiningValue(Valu
>> Value *def = CI->stripPointerCasts();
>> assert(def->getType()->isPointerTy() &&
>> "Base for pointer must be another pointer");
>> - if (isa<CastInst>(def)) {
>> - // If we find a cast instruction here, it means we've found a cast
>> - // which is not simply a pointer cast (i.e. an inttoptr). We don't
>> - // know how to handle int->ptr conversion.
>> - llvm_unreachable("Can not find the base pointers for an inttoptr
>> cast");
>> - }
>> + // If we find a cast instruction here, it means we've found a cast
>> which is
>> + // not simply a pointer cast (i.e. an inttoptr). We don't know how
>> to
>> + // handle int->ptr conversion.
>> assert(!isa<CastInst>(def) && "shouldn't find another cast here");
>> return findBaseDefiningValue(def);
>> }
>> @@ -492,13 +488,8 @@ static Value *findBaseDefiningValue(Valu
>> if (SelectInst *select = dyn_cast<SelectInst>(I)) {
>> return select;
>> }
>> - if (PHINode *phi = dyn_cast<PHINode>(I)) {
>> - return phi;
>> - }
>> - errs() << "unknown type: " << *I << "\n";
>> - llvm_unreachable("unknown type");
>> - return nullptr;
>> + return cast<PHINode>(I);
>>
> 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.
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.
>
> }
>> /// Returns the base defining value for this value.
>> @@ -635,18 +626,18 @@ private:
>> case PhiState::Base:
>> assert(stateA.getBase() && "can't be null");
>> - if (stateB.isUnknown()) {
>> + if (stateB.isUnknown())
>> return stateA;
>> - } else if (stateB.isBase()) {
>> +
>> + if (stateB.isBase()) {
>> if (stateA.getBase() == stateB.getBase()) {
>> assert(stateA == stateB && "equality broken!");
>> return stateA;
>> }
>> return PhiState(PhiState::Conflict);
>> - } else {
>> - assert(stateB.isConflict() && "only three states!");
>> - return PhiState(PhiState::Conflict);
>> }
>> + assert(stateB.isConflict() && "only three states!");
>> + return PhiState(PhiState::Conflict);
>> case PhiState::Conflict:
>> return stateA;
>> @@ -701,10 +692,9 @@ static Value *findBasePointer(Value *I,
>> Value *v = Pair.first;
>> assert(!isKnownBaseResult(v) && "why did it get added?");
>> if (PHINode *phi = dyn_cast<PHINode>(v)) {
>> - unsigned NumPHIValues = phi->getNumIncomingValues();
>> - assert(NumPHIValues > 0 && "zero input phis are illegal");
>> - for (unsigned i = 0; i != NumPHIValues; ++i) {
>> - Value *InVal = phi->getIncomingValue(i);
>> + assert(phi->getNumIncomingValues() > 0 &&
>> + "zero input phis are illegal");
>> + for (Value *InVal : phi->incoming_values()) {
>> Value *local = findBaseOrBDV(InVal, cache);
>> if (!isKnownBaseResult(local) && states.find(local) ==
>> states.end()) {
>> states[local] = PhiState();
>> @@ -748,18 +738,12 @@ static Value *findBasePointer(Value *I,
>> MeetPhiStates calculateMeet(states);
>> Value *v = Pair.first;
>> assert(!isKnownBaseResult(v) && "why did it get added?");
>> - assert(isa<SelectInst>(v) || isa<PHINode>(v));
>> if (SelectInst *select = dyn_cast<SelectInst>(v)) {
>> calculateMeet.meetWith(findBaseOrBDV(select->getTrueValue(),
>> cache));
>> calculateMeet.meetWith(findBaseOrBDV(select->getFalseValue(),
>> cache));
>> - } else if (PHINode *phi = dyn_cast<PHINode>(v)) {
>> - for (unsigned i = 0; i < phi->getNumIncomingValues(); i++) {
>> - calculateMeet.meetWith(
>> - findBaseOrBDV(phi->getIncomingValue(i), cache));
>> - }
>> - } else {
>> - llvm_unreachable("no such state expected");
>> - }
>> + } else
>> + for (Value *Val : cast<PHINode>(v)->incoming_values())
>> + calculateMeet.meetWith(findBaseOrBDV(Val, cache));
>> PhiState oldState = states[v];
>> PhiState newState = calculateMeet.getResult();
>> @@ -989,13 +973,13 @@ static void findBasePointers(const State
>> cast<Instruction>(ptr)->getParent())) &&
>> "The base we found better dominate the derived pointer");
>> - if (isNullConstant(base))
>> - // If you see this trip and like to live really dangerously, the
>> code
>> - // should be correct, just with idioms the verifier can't handle.
>> You
>> - // can try disabling the verifier at your own substaintial risk.
>> - llvm_unreachable("the relocation code needs adjustment to handle
>> the"
>> - "relocation of a null pointer constant without
>> causing"
>> - "false positives in the safepoint ir verifier.");
>> + // If you see this trip and like to live really dangerously, the
>> code should
>> + // be correct, just with idioms the verifier can't handle. You can
>> try
>> + // disabling the verifier at your own substaintial risk.
>> + assert(!isNullConstant(base) && "the relocation code needs
>> adjustment to "
>> + "handle the relocation of a null
>> pointer "
>> + "constant without causing false
>> positives "
>> + "in the safepoint ir verifier.");
>> }
>> }
>> @@ -1258,7 +1242,7 @@ makeStatepointExplicitImpl(const CallSit
>> Builder.SetInsertPoint(IP);
>> Builder.SetCurrentDebugLocation(IP->getDebugLoc());
>> - } else if (CS.isInvoke()) {
>> + } else {
>> InvokeInst *toReplace = cast<InvokeInst>(CS.getInstruction());
>> // Insert the new invoke into the old block. We'll remove the
>> old one in a
>> @@ -1309,8 +1293,6 @@ makeStatepointExplicitImpl(const CallSit
>> // gc relocates will be generated later as if it were regular call
>> // statepoint
>> - } else {
>> - llvm_unreachable("unexpect type of CallSite");
>> }
>> assert(token);
>> @@ -1526,12 +1508,11 @@ static void relocationViaAlloca(
>> if (auto II = dyn_cast<InvokeInst>(Statepoint)) {
>> InsertClobbersAt(II->getNormalDest()->getFirstInsertionPt());
>> InsertClobbersAt(II->getUnwindDest()->getFirstInsertionPt());
>> - } else if (auto CI = dyn_cast<CallInst>(Statepoint)) {
>> - BasicBlock::iterator Next(CI);
>> + } else {
>> + BasicBlock::iterator Next(cast<CallInst>(Statepoint));
>> Next++;
>> InsertClobbersAt(Next);
>> - } else
>> - llvm_unreachable("illegal statepoint instruction type?");
>> + }
>> #endif
>> }
>> // update use with load allocas and add store for gc_relocated
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150220/f5a557e9/attachment.html>
More information about the llvm-commits
mailing list