[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1050-1051
+    // NOTE: if it is unsigned, then the comparison is naturally always 'false'.
+    llvm::ICmpInst::Predicate Pred =
+        VSigned ? llvm::ICmpInst::ICMP_SLT : llvm::ICmpInst::ICMP_ULT;
+    // Get the zero of the same type with which we will be comparing.
----------------
lebedev.ri wrote:
> rsmith wrote:
> > lebedev.ri wrote:
> > > rsmith wrote:
> > > > If `!VSigned`, the result is a constant `false`; you don't need to emit an `icmp` to work that out.
> > > Ok, if you insist.
> > > I didn't do that in the first place because we will now have an `icmp`
> > > where one operand being a constant, so we can simplify it further.
> > > And i don't want to complicate this logic if middle-end already handles it :)
> > This becomes a lot simpler with the approach I described in the other comment thread, because you don't need a second `icmp eq` at all.
> Humm. So i have initially did this. It is probably broken for non-scalars, but we don't care probably.
> 
> But then i thought more.
> 
> If we do not emit truncation check, we get `icmp eq (icmp ...), false`, which is tautological.
> We can't just drop the outer `icmp eq` since we'd get [[ https://rise4fun.com/Alive/4slv | the opposite value ]].
> We could emit `xor %icmp, -1` to invert it.  Or simply invert the predicate, and avoid the second `icmp`.
> By itself, either of these options doesn't sound that bad.
> 
> But if both are signed, we can't do that. So we have to have two different code paths...
> 
> If we do emit the `icmp ult %x, 0`, [it naturally works with vectors], we avoid complicating the front-end,
> and the middle-end playfully simplifies this IR with no sweat.
> 
> So why do we want to complicate the front-end //in this case//, and not let the middle-end do it's job?
> I'm unconvinced, and i have kept this as is. :/
> If !VSigned, the result is a constant false; you don't need to emit an icmp to work that out.

Even at `-O0`, dagcombine constant-folds (unsurprizingly) this case, https://godbolt.org/z/D5ueOq


Repository:
  rC Clang

https://reviews.llvm.org/D50250