[PATCH] D85593: [InstCombine] ~(~X + Y) -> X - Y

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/not-add.ll:94
+; CHECK-NEXT:    [[A:%.*]] = add i8 [[NOTX]], [[Y:%.*]]
+; CHECK-NEXT:    call void @use(i8 [[A]])
+; CHECK-NEXT:    [[NOTA:%.*]] = xor i8 [[A]], -1
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > nikic wrote:
> > > lebedev.ri wrote:
> > > > nikic wrote:
> > > > > xbolva00 wrote:
> > > > > > spatel wrote:
> > > > > > > xbolva00 wrote:
> > > > > > > > @lebedev.ri
> > > > > > > > 
> > > > > > > > not seems better than sub x, y so oneuse check is ok (?)
> > > > > > > It's true that in some cases (for example, expensive FP ops), we have leaned toward a more restrictive one-use check.
> > > > > > > 
> > > > > > > In this case, however, I do not think that the relative analysis improvement of an xor vs. sub should limit the transform. 
> > > > > > > 
> > > > > > > Reducing the number of operand uses and dependency chain could allow follow-on improvements.
> > > > > > Ok, thanks. Will drop it.
> > > > > Is this transform even legal without the one-use restriction? It *increases* the number of uses, as such it's not obvious that it the transform without one-use restriction is undef-safe.
> > > > > Is this transform even legal without the one-use restriction? It *increases* the number of uses, as such it's not obvious that it the transform without one-use restriction is undef-safe.
> > > > 
> > > > Increased instruction count is only a legality issue
> > > > if it is increased for the computation of the value,
> > > > which isn't the case here.
> > > > Otherwise basically every instcombine fold would be invalid.
> > > > ```
> > > > ----------------------------------------
> > > > define i8 @src(i8 %x, i8 %y) {
> > > > %0:
> > > >   %notx = xor i8 %x, 255
> > > >   %a = add i8 %notx, %y
> > > >   %nota = xor i8 %a, 255
> > > >   call void @use(i8 %notx)
> > > >   call void @use(i8 %a)
> > > >   ret i8 %nota
> > > > }
> > > > =>
> > > > define i8 @tgt(i8 %x, i8 %y) {
> > > > %0:
> > > >   %notx = xor i8 %x, 255
> > > >   %a = add i8 %notx, %y
> > > >   %s = sub i8 %x, %y
> > > >   call void @use(i8 %notx)
> > > >   call void @use(i8 %a)
> > > >   ret i8 %s
> > > > }
> > > > Transformation seems to be correct!
> > > > 
> > > > ```
> > > After all this time, I'm probably still confused about undef semantics :)
> > > 
> > > Let me try to explain what I am concerned about: To simplify your example, let's say that `%x = 0` and `%y = undef`. This leaves us with:
> > > 
> > > ```
> > > define i8 @src() {
> > >   %a = add i8 255, undef
> > >   %nota = xor i8 %a, 255
> > >   call void @use(i8 %a)
> > >   ret i8 %nota
> > > }
> > > =>
> > > define i8 @tgt(i8 %y) {
> > >   %a = add i8 255, undef
> > >   %s = sub i8 0, undef
> > >   call void @use(i8 %a)
> > >   ret i8 %s
> > > }
> > > ```
> > > 
> > > For `@src`, if we set `undef=0`, then `%a = 255` and `%nota = 0`. In `@tgt`, we have two separate undefs that can be chosen independently, so we could end up with `%a = 255` and `%nota = 255` for example.
> > > 
> > > Is my interpretation incorrect? I've seen this issue come up in the past with transforms that increase the number of uses of values, but maybe I'm missing an important distinction here (e.g. is the fact that `add 255, undef` can be folded to `undef` relevant here? Do we assume that such a fold must happen?)
> > > 
> > > I don't think alive can be used to check multi-use correctness currently due to some implementation issue (https://github.com/AliveToolkit/alive2/issues/450).
> > Yeah, but there must be some rules otherwise almost all folds would be invalid.
> cc @aqjune
While I would like to understand this, I also want to say that this shouldn't block this patch. Feel free to go ahead :)


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

https://reviews.llvm.org/D85593