efriedma added a comment. > This fails with the new asm constraint checks, since the dead branch is never > pruned.
As far as I know, we didn't touch "i", only "n". Is there a bug filed for the issue you're describing? ================ Comment at: clang/lib/CodeGen/CGStmt.cpp:1852 + IntResult = + llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity()); + else ---------------- hans wrote: > efriedma wrote: > > This always returns an APSInt with width 64; is that really right? I guess > > it might not really matter given that it's only going to be used as an > > immediate constant anyway, but it seems weird. > I agree it seems a little strange, but I think in practice it's correct. > EVResult.Val.getLValueOffset().getQuantity() returns an int64_t, so we're not > losing any data. > > The code that I lifted this from, is using the bitwidth of the casted-to > integer type for the result. But it's still only got maximum 64 bits since > the source, getLValueOffset().getQuantity(), is the same. The concern isn't that we would lose data. I'm more concerned the backend might not be prepared for a value of the "wrong" width. ================ Comment at: clang/lib/Sema/SemaStmtAsm.cpp:399 + IntResult = + llvm::APSInt::get(EVResult.Val.getLValueOffset().getQuantity()); + else ---------------- hans wrote: > efriedma wrote: > > I think it makes sense to add a method to APValue specifically to do the > > conversion from LValue to an APSInt, whether or not isNullPointer() is > > true, and use it both here and in IntExprEvaluator::VisitCastExpr in > > lib/AST/ExprConstant.cpp. The logic is sort of subtle (and I'm not > > completely sure it's right for targets where null is not zero, but you > > shouldn't try to fix that here). > I agree (and this was also Bill's suggestion above) that it would be nice to > have a utility method for this. > > I'm not sure adding one to APValue would work for > IntExprEvaluator::VisitCastExpr though, since that code is actually using its > own LValue class, not an APValue until it's time to return a result. > > I frankly also doesn't fully understand what that code is doing. If the > LValue has a base value, it seems to just take that as result and ignore any > offset? This is unknown territory to me, but the way I read it, if there's an > lvalue base, the expression isn't going to come out as an integer constant. I > think. > > About null pointers, I'm calling getTargetNullPointerValue() so I think that > should be okay, no? Oh, I didn't realize IntExprEvaluator::VisitCastExpr wasn't using the same class to represent the value; that makes it harder to usefully refactor. But still, it would be good to reduce the duplicated code between here and CodeGen. > If the LValue has a base value, it seems to just take that as result and > ignore any offset? If there's a base value, it returns the whole LValue, including the base and offset. > I'm calling getTargetNullPointerValue() so I think that should be okay The issue would be the case where you have a null pointer with an offset, like the case in the bug. It's sort of inconsistent if null==-1, but null+1==1. But it's not something we handle consistently elsewhere, anyway, so I guess we can ignore it for now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58821/new/ https://reviews.llvm.org/D58821 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
