Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector related arith missing nsw on addition #664

Open
bcardosolopes opened this issue Jun 6, 2024 · 2 comments
Open

Vector related arith missing nsw on addition #664

bcardosolopes opened this issue Jun 6, 2024 · 2 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@bcardosolopes
Copy link
Member

If we look at how LLVM currently codegen for this, we get a add nsw instead of a plain add, see https://godbolt.org/z/xdevh38r8. There are probably other differences too.

Originally posted by @bcardosolopes in #613 (comment)

@seven-mile
Copy link
Collaborator

I can repro this for just scalar arith, too. See this program. I guess we are missing the corresponding logic in ScalarExprEmitter. The lowering is correct however.

if (op.Ty->isSignedIntegerOrEnumerationType()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
return Builder.CreateAdd(op.LHS, op.RHS, "add");
[[fallthrough]];
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
[[fallthrough]];
case LangOptions::SOB_Trapping:
if (CanElideOverflowCheck(CGF.getContext(), op))
return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
return EmitOverflowCheckedBinOp(op);
}
}

@mingshi2333
Copy link
Contributor

I checked and it seems Subop has already done this logic, so if no one else has done it, it can be assigned to me

mlir::Value ScalarExprEmitter::buildSub(const BinOpInfo &Ops) {
// The LHS is always a pointer if either side is.
if (!Ops.LHS.getType().isa<mlir::cir::PointerType>()) {
if (Ops.CompType->isSignedIntegerOrEnumerationType()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined: {
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
return Builder.createSub(Ops.LHS, Ops.RHS);
[[fallthrough]];
}
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
return Builder.createNSWSub(Ops.LHS, Ops.RHS);
[[fallthrough]];
case LangOptions::SOB_Trapping:
if (CanElideOverflowCheck(CGF.getContext(), Ops))
return Builder.createNSWSub(Ops.LHS, Ops.RHS);
llvm_unreachable("NYI");
}
}
if (Ops.FullType->isConstantMatrixType()) {

bcardosolopes pushed a commit that referenced this issue Jun 20, 2024
…perator (#677)

This PR is to fix the missing **nsw** flag in issue #664 regarding add,
mul arithmetic operations. there is also a problem with unary operations
such as **Inc ,Dec,Plus,Minus and Not** . which should also have 'nsw'
flag [example](https://godbolt.org/z/q3o3jsbe1). This part should need
to be fixed through lowering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants