[PATCH] D24327: [DAGCombine] Modification of visitBR_CC

[Sanjay Patel via llvm-commits]

spatel added subscribers: sanjoy, majnemer.
spatel added a comment.

In https://reviews.llvm.org/D24327#537999, @bryant wrote:

> In https://reviews.llvm.org/D24327#537057, @spatel wrote:
>
> > > %2 = add %1, #const
> >
> > >  br_cc cond %2, #const
> >
> > >  -->
> >
> > >  br_cc cond %1, 0
> >
> >
> > I don't think this transform is safe unless you have 'nsw' or 'nuw' on the add.
>
>
> am i missing something obvious? if nsw/nuw are present, then %2 could potentially be undef, and the transform would elide this case and alter semantics. so wouldn't it be unsafe only if the flags are present?


The wrapping flags are what allow us to guarantee that the transform does not alter semantics.

Consider the example with these values:

  %1 is i8 255 (unsigned max)
  %2 = add i8 %1, 1
  br_cc ugt %2, 1

Wrapping is allowed; there is no undef. 
The branch condition is:

  ugt 0, 1 --> false.

If we make the transform suggested by this patch, the branch condition is:

  ugt 255, 0  --> true

If the add has 'nuw', this case cannot occur. Otherwise, we have poison:
http://llvm.org/docs/LangRef.html#poisonvalues

Note that the semantics of DAG instructions are not actually specified/defined anywhere AFAIK (if they are, I'd love to know where!). I'm assuming they are the same as LLVM IR in this case because we can and do pass nsw/nuw flags from IR instructions to DAG nodes.

Please take a look at how this transform is handled for signed and unsigned compares in InstCombineCompares.cpp. I assume the reason we need this fold repeated as a DAG combine is that the pattern does not emerge until it's too late to be handled by InstCombine?

cc'ing Sanjoy and David in case I have any or all of this wrong. :)


https://reviews.llvm.org/D24327