[CIR][CodeGen][Lowering] Support Integer overflow with fwrap

[gitoleg]

This PR fixes some cases when a program compiled with -fwrapv fails with NYI . Basically, the default behavior is no overlap:

void baz(int x, int y) {
  int z = x - y;
}

LLVM IR (no CIR enabled):

%sub = sub nsw i32 %0, %1

and with -fwrapv :

%sub = sub i32 %0, %1

We need something similar in CIR. The only way I see how to implement it is to add a couple of attributes to the BinOp to make things even with the llvm dialect.

Well, are there any other ideas?

[Lancern]

Since nsw and nuw only applies to integral operands, what about inventing a new operation specifically for wrapped integral arithmetic? Something like cir.binop.wrapped.

[gitoleg]

@Lancern

Since nsw and nuw only applies to integral operands, what about inventing a new operation specifically for wrapped integral arithmetic? Something like cir.binop.wrapped.

well, maybe .. but I would not introduce new operations for math arithmetic, at least now, if there is a chance to do the same with the existing ones.

@bcardosolopes I didn't have a chance to check carefully, but as far I understand unsigned wrap is a standard behavior in C, and only in the rare case we need to emphasize there is no wrap with nuw. So from my point of view (at least now) it's may be wrong way to infer "nuw" or "nsw" from integer type. Or even more! I took a look at original codegen tests, and it could be both nuw nsw set simultaneously!

[bcardosolopes]

Mistake from my review: my intention was to suggest nowrap, not wrap. Thanks for the feedback @gitoleg

well, maybe .. but I would not introduce new operations for math arithmetic, at least now, if there is a chance to do the same with the existing ones.

+1

I didn't have a chance to check carefully, but as far I understand unsigned wrap is a standard behavior in C, and only in the rare case we need to emphasize there is no wrap with nuw. So from my point of view (at least now) it's may be wrong way to infer "nuw" or "nsw" from integer type. Or even more! I took a look at original codegen tests, and it could be both nuw nsw set simultaneously!

We need testcases that cover that too.

By looking at the PR, seems like this is a TU level configuration, and emitting these flags is dependent on:

• CGF.getLangOpts().isSignedOverflowDefined() being ON
• The type of overflow behavior: SOB_Defined, SOB_Undefined and SOB_Trapping.

So here's one idea, perhaps we should encode that at the module level (like we for fp behavior) and LLVM lowering could then look at these module properties to pick how it should lower BinOps. How does that sound? For this to work, I'm assuming that all of these binops on both signed and unsigned in a TU will have to follow the rules, do you know if that applies?

[bcardosolopes]

In fact we already have those global definitions, see SignedOverflowBehaviorAttr.

[gitoleg]

@bcardosolopes So far I can't prove to myself that we can solve it on the module level, since there are both NUW and NSW arithmetic instructions emitted in the original codegen even without language options checked. So I'm afraid we have to do it for operations :(

We need testcases that cover that too.

Just if you are curious about, how it's possible:

unsigned test(unsigned x, unsigned y) {
  return x << y;
}

compiled with -fsanitize=unsigned-shift-base So I think we need to support this sanitizer first.

---

NUW - no unsigned wrap NSW - no signed wrap

[gitoleg]

@bcardosolopes

Now the question is - do you want me to do the same for ShiftOp (for the left one) in this PR? OR we just postpone it until we will need it? I just noticed shifts are not BinOp in cir.

[bcardosolopes]

Now the question is - do you want me to do the same for ShiftOp (for the left one) in this PR? OR we just postpone it until we will need it? I just noticed shifts are not BinOp in cir.

It'd be great if you can already fix the shift, so we don't need to revisit this - feel free to do it in another PR though, this one does good incremental work.

[gitoleg]

@bcardosolopes done!