Changeset View
Standalone View
lib/Transforms/InstCombine/InstCombineCasts.cpp
| Show First 20 Lines • Show All 271 Lines • ▼ Show 20 Lines | if (Instruction::CastOps NewOpc = isEliminableCastPair(CSrc, &CI)) { | ||||
| // cast pointing to the new Value. | // cast pointing to the new Value. | ||||
| SmallVector<DbgInfoIntrinsic *, 1> CSrcDbgInsts; | SmallVector<DbgInfoIntrinsic *, 1> CSrcDbgInsts; | ||||
| findDbgUsers(CSrcDbgInsts, CSrc); | findDbgUsers(CSrcDbgInsts, CSrc); | ||||
| if (CSrcDbgInsts.size()) { | if (CSrcDbgInsts.size()) { | ||||
| DIBuilder DIB(*CI.getModule()); | DIBuilder DIB(*CI.getModule()); | ||||
| for (auto *DII : CSrcDbgInsts) | for (auto *DII : CSrcDbgInsts) | ||||
| DIB.insertDbgValueIntrinsic( | DIB.insertDbgValueIntrinsic( | ||||
| Res, DII->getVariable(), DII->getExpression(), | Res, DII->getVariable(), DII->getExpression(), | ||||
| DII->getDebugLoc().get(), &*std::next(CI.getIterator())); | DII->getDebugLoc().get(), &*std::next(CI.getIterator())); | ||||
vsk: There should be a helper function that cuts down on the boilerplate needed to insert fresh… | |||||
| } | } | ||||
| return Res; | return Res; | ||||
| } | } | ||||
| } | } | ||||
| if (auto *Sel = dyn_cast<SelectInst>(Src)) { | if (auto *Sel = dyn_cast<SelectInst>(Src)) { | ||||
| // We are casting a select. Try to fold the cast into the select, but only | // We are casting a select. Try to fold the cast into the select, but only | ||||
| // if the select does not have a compare instruction with matching operand | // if the select does not have a compare instruction with matching operand | ||||
| ▲ Show 20 Lines • Show All 793 Lines • ▼ Show 20 Lines | LLVM_DEBUG( | ||||
| dbgs() << "ICE: EvaluateInDifferentType converting expression type" | dbgs() << "ICE: EvaluateInDifferentType converting expression type" | ||||
| " to avoid zero extend: " | " to avoid zero extend: " | ||||
| << CI << '\n'); | << CI << '\n'); | ||||
| Value *Res = EvaluateInDifferentType(Src, DestTy, false); | Value *Res = EvaluateInDifferentType(Src, DestTy, false); | ||||
| assert(Res->getType() == DestTy); | assert(Res->getType() == DestTy); | ||||
| uint32_t SrcBitsKept = SrcTy->getScalarSizeInBits()-BitsToClear; | uint32_t SrcBitsKept = SrcTy->getScalarSizeInBits()-BitsToClear; | ||||
| uint32_t DestBitSize = DestTy->getScalarSizeInBits(); | uint32_t DestBitSize = DestTy->getScalarSizeInBits(); | ||||
Nit -- please capitalize this and use the same line wrapping as the surrounding code. vsk: Nit -- please capitalize this and use the same line wrapping as the surrounding code. | |||||
| // Since the old instruction is merged, we preserve it's DI as | |||||
| // a fragment in the resulting instruction | |||||
vskUnsubmitted Not Done ReplyInline ActionsCould you make the comment more specific? Example: "Preserve any debug values referring to the zext by ...". vsk: Could you make the comment more specific? Example: "Preserve any debug values referring to the… | |||||
Not Done ReplyInline ActionsComment should end with a full-stop. Also it's should be its here. probinson: Comment should end with a full-stop. Also `it's` should be `its` here. | |||||
| SmallVector<DbgInfoIntrinsic *, 1> SrcDbgInsts; | |||||
| findDbgUsers(SrcDbgInsts, Src); | |||||
| if (SrcDbgInsts.size()) { | |||||
| DIBuilder DIB(*CI.getModule()); | |||||
| for (auto *DII : SrcDbgInsts) { | |||||
| auto Fragment = DIExpression::createFragmentExpression( | |||||
DIExpression doesn't need to be null-checked here because DbgValueInst::getExpression() is always nonnull. vsk: DIExpression doesn't need to be null-checked here because DbgValueInst::getExpression() is… | |||||
| DII->getExpression(), SrcBitsKept, DestBitSize); | |||||
vskUnsubmitted Not Done ReplyInline Actions+ @aprantl and @probinson for dwarf expertise. There are two cases: a zext can either operate on integers or on vectors of integers. I think the fragment you're creating here is correct in the first case, but not the second. For the second case, I think you'd to emit multiple dbg.values with distinct fragments describing each component of the original vector. vsk: + @aprantl and @probinson for dwarf expertise.
There are two cases: a zext can either operate… | |||||
probinsonUnsubmitted Not Done ReplyInline ActionsEmitting one fragment per element sounds plausible, but Adrian is really the fragment expert. probinson: Emitting one fragment per element sounds plausible, but Adrian is really the fragment expert. | |||||
vskUnsubmitted Not Done ReplyInline ActionsWith the helper I suggested, the zext-of-integer case could be simplified to: InsertReplacementDebugValues(Src, Res, &*std::next(CI.getIterator()), [&](DbgInfoIntrinsic &OldDII) -> DIExpression * {
return DIExpression::createFragmentExpression(OldDII.getExpression(), SrcBitsKept, DestBitSize);
});vsk: With the helper I suggested, the zext-of-integer case could be simplified to:
```… | |||||
You mentioned offline that this causes a verifier failure. If we can't use fragments, consider using a simpler approach for scalars: using an empty expression. It won't work for vectors, but in the scalar case, the high bits should already be cleared. We can relax the check in https://reviews.llvm.org/D48408 to allow this. vsk: You mentioned offline that this causes a verifier failure. If we can't use fragments, consider… | |||||
Not Done ReplyInline ActionsIf I use an empty DIExpression it doesn't change the fact that the dbg.value call points to a DILocalVariable that after instcombine has a different DIBasicType than what it used to. Basiacly the dbg.value call says that the value was changed to an i32 but the it's type is still i8 in the debug info. If there is a way to explain this to the debugger using DIExpressions, I can't find it. gramanas: If I use an empty DIExpression it doesn't change the fact that the dbg.value call points to a… | |||||
Not Done ReplyInline Actions(Not sure if that is your question) We don't have any means to express that the SSA value used by a dbg.value instrinsic is larger (e.g., because of s/zext) than the size of the fragment being described. For example, if the size of the variable is 8 bits and the value is 32 bits then implicitly the lower 8 bits are used. You could make this explicit by masking out everything but the lower out bits in the DIExpression (DW_OP_constu 0xff DW_OP_and) but this is redundant so we don't do it. Why would you like to express this? aprantl: (Not sure if that is your question) We don't have any means to express that the SSA value used… | |||||
Not Done ReplyInline ActionsI thought the fact that a 32 bit variable has DIBasicType of size 8 is wrong. But modifying the DI would interfere with the way the front-end described the program at the first place, thus I was thinking of a way to describe it with a DIExpression. Since the correct value is being displayed implicitly, I guess it's not a problem. gramanas: I thought the fact that a 32 bit variable has DIBasicType of size 8 is wrong. But modifying the… | |||||
Not Done ReplyInline ActionsWhen you say "32 bit variable" are you referring to the source variable or the SSA value that is used by the dbg.value intrinsic? aprantl: When you say "32 bit variable" are you referring to the source variable or the SSA value that… | |||||
Not Done ReplyInline ActionsWhen running opt -debugify -instcombine to the scalar test below, the zext is replaced with a wider mul (i32 from i8) and the resulting dbg.value from this patch points to a DILocalVariable that has DIBasicType of size 8, while its an i32 value. I think I mean the SSA value. gramanas: When running `opt -debugify -instcombine` to the scalar test below, the `zext` is replaced with… | |||||
Not Done ReplyInline ActionsIt looks like it's OK for a integer dbg.value operand to be wider than the type of its associated DILocalVariable. Adrian's second-to-last comment states that the debugger will handle this by displaying the lower bits. vsk: It looks like it's OK for a integer dbg.value operand to be wider than the type of its… | |||||
Not Done ReplyInline ActionsCorrect. I should also point out that the following is entirely legal and actually gets emitted by LLVM on i386 for an 8-bit variable in the ah register: dbg.value(!metadata i32 %value, !metadata "my8bit-variable", !metadata !DIExpression(DW_OP_shr 8)) // = %value >> 8 So we describe an 8-bit variable in bits 8-15 of a. 32-bit value. aprantl: Correct. I should also point out that the following is entirely legal and actually gets emitted… | |||||
| DIB.insertDbgValueIntrinsic( | |||||
| Res, DII->getVariable(), Fragment.getValue(), | |||||
| DII->getDebugLoc().get(), &*std::next(CI.getIterator())); | |||||
This unwraps an Optional without a check. If you're sure this can't fail, please add a comment explaining why. vsk: This unwraps an Optional without a check. If you're sure this can't fail, please add a comment… | |||||
| } | |||||
| } | |||||
| // If the high bits are already filled with zeros, just replace this | // If the high bits are already filled with zeros, just replace this | ||||
| // cast with the result. | // cast with the result. | ||||
| if (MaskedValueIsZero(Res, | if (MaskedValueIsZero( | ||||
| APInt::getHighBitsSet(DestBitSize, | Res, APInt::getHighBitsSet(DestBitSize, DestBitSize - SrcBitsKept), | ||||
gramanasAuthorUnsubmitted Not Done ReplyInline ActionsThis and the following were changed by clang-format, I hope there is no problem formating those, since my work is done nearby. gramanas: This and the following were changed by clang-format, I hope there is no problem formating those… | |||||
vskUnsubmitted This does pose a problem. Please avoid re-formatting code unrelated to your patch. This makes diffs harder to read, and makes attribution harder when looking at git history. If you'd like, you can use the clang-format-diff git hook to only reformat that changes you're making. A simple way to remove these changes from your patch is to upstage your changes, then use git add -p to re-stage the chunks you want to keep. vsk: This does pose a problem. Please avoid re-formatting code unrelated to your patch. This makes… | |||||
aprantlUnsubmitted Not Done ReplyInline ActionsIf you are going to make a lot of changes to the same file, you can also do a separate commit that is just a clang-format of this file ahead of time. aprantl: If you are going to make a lot of changes to the same file, you can also do a separate commit… | |||||
| DestBitSize-SrcBitsKept), | |||||
| 0, &CI)) | 0, &CI)) | ||||
| return replaceInstUsesWith(CI, Res); | return replaceInstUsesWith(CI, Res); | ||||
| // We need to emit an AND to clear the high bits. | // We need to emit an AND to clear the high bits. | ||||
| Constant *C = ConstantInt::get(Res->getType(), | Constant *C = ConstantInt::get( | ||||
| APInt::getLowBitsSet(DestBitSize, SrcBitsKept)); | Res->getType(), APInt::getLowBitsSet(DestBitSize, SrcBitsKept)); | ||||
| return BinaryOperator::CreateAnd(Res, C); | return BinaryOperator::CreateAnd(Res, C); | ||||
| } | } | ||||
| // If this is a TRUNC followed by a ZEXT then we are dealing with integral | // If this is a TRUNC followed by a ZEXT then we are dealing with integral | ||||
| // types and if the sizes are just right we can convert this into a logical | // types and if the sizes are just right we can convert this into a logical | ||||
| // 'and' which will be much cheaper than the pair of casts. | // 'and' which will be much cheaper than the pair of casts. | ||||
| if (TruncInst *CSrc = dyn_cast<TruncInst>(Src)) { // A->B->C cast | if (TruncInst *CSrc = dyn_cast<TruncInst>(Src)) { // A->B->C cast | ||||
| // TODO: Subsume this into EvaluateInDifferentType. | // TODO: Subsume this into EvaluateInDifferentType. | ||||
| ▲ Show 20 Lines • Show All 1,287 Lines • Show Last 20 Lines | |||||
There should be a helper function that cuts down on the boilerplate needed to insert fresh debug values.
Here's the usage I envision:
InsertReplacementDebugValues(CSrc, Res, &*std::next(CI.getIterator()), [](DbgInfoIntrinsic &OldDII) -> DIExpression * { return OldDII.getExpression(); });@aprantl, @mattd and others: wdyt?