On Thu, Jul 20, 2017 at 11:39 AM, Alexander Monakov <amona...@ispras.ru> wrote: > On Thu, 20 Jul 2017, Richard Biener wrote: >> >> So for saturating types isn't the issue when @1 and @2 have opposite >> >> sign and the inner multiply would have saturated? >> > >> > No, I think the only special case is @1 == @2 == -1, otherwise either @2 is >> > 0 or 1, or @1 * @2 is larger in magnitude than @1 (unless @1 == 0), and >> > folding doesn't conceal an intermediate saturation. >> >> Ok, but you don't check for @1 == @2 == -1 because you rely on >> the subtree being folded > > Hm, I don't see why you say that, the patch ensures that @2 != -1 (I agree > with > Marc that implicitly relying on other folding to happen is not quite ok)
But not @1 == -1, so not both. > (for the avoidance of doubt, "the only special case" was meant in the > context of saturating types here) > >> fixed-point math has other issues (but I think at least >> has symmetric negative/positive ranges). > > I don't think so, n1169.pdf calls out that fixed-point _Fract types may have > asymmetric range such that -1.0 is exactly representable but 1.0 is not. I stand corrected then. >> >> > I think overflow in the constant multiplication is ok unless >> >> > TYPE_OVERFLOW_SANITIZED >> >> > (and can we have a ubsan testcase for that that would otherwise fail?). >> >> > It's not that we introduce new overflow here. >> > >> > This is to allow deducing that X must be zero. Otherwise, exploiting >> > undefined overflow allows to fold X * CST1 * CST2 to just 0 if CST1 * CST2 >> > overflows (this is what Marc originally suggested), but shouldn't we >> > rather keep X in the IR to later deduce that X is zero? >> >> I don't think we should care here. After all we could immediately fold >> X * CST1 * CST2 to zero if CST1 * CST2 overflows and overflow invokes >> undefined behavior, right in this reassoc pattern. > > Note we cannot simply check if CST1 * CST2 overflows, because in case their > exact product is -INT_MIN (i.e. INT_MAX+1), the original expression doesn't > overflow for X==-1 (except when CST1==INT_MIN && CST2==-1). This was > previously > mentioned by Marc; sorry for neglecting to mention this special case in my > previous email. > > So the motivation to not to do the transform on overflow is twofold: > > - unclear how to handle CST1*CST2 == -INT_MIN, if at all; > - otherwise, folding X*CST1*CST2 to literal zero would drop X from the > IR; but if X is an SSA_NAME, we could instead deduce that X is zero and > replace all dominated uses of X by zero (I don't know how > real-world-relevant > this is) > >> So -- I'd prefer to just give up for TYPE_SATURATING (as you say currently >> it can't happen anyway because the only saturating types we have are >> fixed-point), >> thus remove the zero/minus_one checks (not worry about false positive in >> ubsan). > > OK, I think that can be done (I assume you mean 'false negative in ubsan'), > but in light of the above clarification, what change do you want for overflow? Ok, let's leave overflow handling as in your patch. It looks like fold_binary associate: case does the same. It does look like an unnecessary restriction but we follow existing practice there. Richard. > Alexander
