[PATCH] D143883: [InstCombine] canonicalize urem as cmp+select

Allen zhong via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 17 17:50:43 PST 2023


Allen marked 5 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:5
+; https://alive2.llvm.org/ce/z/UNmz9j
+define noundef i64 @urem_assume(i64 noundef %x, i64 noundef %n) {
+; CHECK-LABEL: @urem_assume(
----------------
nikic wrote:
> Should also drop noundef here and on the return values.
Thanks, delete all the noundef.


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:43
+
+define noundef i64 @urem_assume_eq(i64 noundef %x, i64 noundef %n) {
+; CHECK-LABEL: @urem_assume_eq(
----------------
spatel wrote:
> If it's not changing behavior of the optimization (and it is not in any cases here from what I see), use i8 or possibly even narrower type widths. This makes it more likely that Alive2 can run quickly on all tests to verify correctness.
Thanks, I adapt all the value type into i8.


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:105
+start:
+  %cmp = icmp ult i64 %x, %n
+  %add = add i64 %x, 1
----------------
nikic wrote:
> efriedma wrote:
> > nikic wrote:
> > > %cmp is a dead instruction here. The request wasn't to just drop the assume, but to establish the `%x < %n` precondition without using one. It's a bit tricky to do this without also triggering other folds, but maybe this would work?
> > > ```
> > > define i64 @test(i64 %arg) {
> > >   %x = shl nuw i64 1, %arg
> > >   %n = shl nuw i64 3, %arg
> > >   %add = add i64 %x, 1
> > >   %out = urem i64 %add, %n
> > >   ret i64 %out
> > > }
> > > ```
> > > Unfortunately this example still doesn't show the need for freeze (https://alive2.llvm.org/ce/z/FJCdt4) ... I'm pretty sure it's needed in the general case, but I'm not sure how to come up with an example.
> > If I'm understanding your question correctly, the following demonstrates the need for freeze.
> > 
> > ```
> > define i8 @src(i8 %arg, i8 %arg2) {
> >   %x = urem i8 %arg, %arg2
> >   %add = add i8 %x, 1
> >   %out = urem i8 %add, %arg2
> >   ret i8 %out
> > }
> > 
> > define i8 @tgt(i8 %arg, i8 %arg2) {
> >   %xx = urem i8 %arg, %arg2
> >   %x = freeze i8 %xx
> >   %add = add i8 %x, 1
> >   %cmp = icmp eq i8 %add, %arg2
> >   %out = select i1 %cmp, i8 0, i8 %add
> >   ret i8 %out
> > }
> > ```
> Thanks, that's exactly what I was looking for!
Thanks @nikic and @efriedma for your test case.


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

https://reviews.llvm.org/D143883



More information about the llvm-commits mailing list