[PATCH] D98410: [ConstantFold] Handle undef/poison when constant folding smul_fix/smul_fix_sat

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/test/Transforms/InstSimplify/ConstProp/smul-fix.ll:148
+  <8 x i3> <i3 undef, i3 2, i3 poison, i3 2, i3 2, i3 2, i3 2, i3 2>,
+  i32 2)
+  ret <8 x i3> %r
----------------
aqjune wrote:
> bjope wrote:
> > aqjune wrote:
> > > aqjune wrote:
> > > > I found from LangRef that actually this can be optimized further:
> > > > ```
> > > > It is undefined behavior if the result value does not fit within the range of the fixed point type.
> > > > ```
> > > > 
> > > > The result of the third element, `llvm.smul.fix.v8i3(2, poison, 2)`, raises UB because `poison` can be folded into a large number which explicitly leads to UB.
> > > > Then, output elements not only 3rd/4th but also others are okay to be folded to `poison`.
> > > > 
> > > > `udiv <x, x>, <0, 1>` is also returning `<undef, undef>` in the same context currently.
> > > > 
> > > > Would this optimization be necessary though? 
> > > > The result of the third element, llvm.smul.fix.v8i3(2, poison, 2), raises UB because poison can be folded into a large number which explicitly leads to UB.
> > > 
> > > Nevermind, I forgot the scale thing.
> > > The first argument is actually 0.5 and the second one is, well, poison / 4, so it is UB if "0.5 * (poison / 4)" can overflow. Is my understanding right?
> > > Then, I think it is uncertain whether this is UB or not.
> > Yes, the example `llvm.smul.fix.i3(2, poison, 2)` is multiplying `poison/4` by `0.5`. That should not raise poison itself (you can't get overflow when multiplying by 0.5). Although,  I've assumed that poison should be propagated here as we multiply be something that is poison.
> > 
> > But you are right that there could be situations when we could detect overflow in constant folding of smul.fix, and thereby introducing poison. For example if having `(smul.fix.i4 -8, -8, 3)` ,  which means that we multiply -1 by -1. The result would be out of range for a 4-bit fixed point value with scale 3 (the largest representable number would be 1/2+1/4+1/8). That has not been consideredi n this patch (focus was to deal with undef/poison already being present in the inputs).
> > As I wrote at line 2808, it might be possible to use APFixedPoint to calculate the result in the future. And I think that API can return a bool indicating if there was overflow. So that could make it pretty simple to deduce if overflow happened or not.
> > 
> > 
> Thank you for the example!
> 
> > Yes, the example llvm.smul.fix.i3(2, poison, 2) is multiplying poison/4 by 0.5. That should not raise poison itself (you can't get overflow when multiplying by 0.5). Although, I've assumed that poison should be propagated here as we multiply be something that is poison.
> 
> Would it be great if this is clarified in LangRef as well? Maybe I can simply add 'if any of the operands is poison, the result is poison' there.
> Thank you for the example!
> 
> > Yes, the example llvm.smul.fix.i3(2, poison, 2) is multiplying poison/4 by 0.5. That should not raise poison itself (you can't get overflow when multiplying by 0.5). Although, I've assumed that poison should be propagated here as we multiply be something that is poison.
> 
> Would it be great if this is clarified in LangRef as well? Maybe I can simply add 'if any of the operands is poison, the result is poison' there.

Isn't this basically described here already https://llvm.org/docs/LangRef.html#poison-values ;  "An instruction that depends on a poison value, produces a poison value itself".
So there is nothing special with smul.fix.* compared to other instructions such as add/mul/xor etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98410/new/

https://reviews.llvm.org/D98410