[PATCH] D109214: [DAG] Fold setcc eq with ashr to compare to zero.

Mikael Holmén via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 8 04:05:49 PDT 2021


uabelho added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3921
+                          DAG.getConstant(0, dl, OpVT),
+                          Cond == ISD::SETEQ ? ISD::SETLT : ISD::SETGT);
+    }
----------------
uabelho wrote:
> dmgreen wrote:
> > foad wrote:
> > > dmgreen wrote:
> > > > uabelho wrote:
> > > > > uabelho wrote:
> > > > > > The comment at line 3914 says we should use "setge" for the "setne" case, but the code actually use "SETGT"?
> > > > > > 
> > > > > > I'm seeing a miscompile with this patch for my out of tree target and I'm not sure what the problem actually is, but the difference between the comment and the code caught my eye.
> > > > > When it goes wrong for me the above code rewrites
> > > > > ```
> > > > >     t22: i32 = sra t20, Constant:i16<31>, foo.c:15:24
> > > > >   t29: i16 = setcc t22, Constant:i32<-1>, setne:ch, foo.c:15:22
> > > > > ```
> > > > > into
> > > > > ```
> > > > >     t36: i16 = setcc t20, Constant:i32<0>, setgt:ch, foo.c:15:22
> > > > > ```
> > > > > t20 is 0, so t22 is also 0 and t29 is 1.
> > > > > But t36 is 0. With "setge" instead of "setgt" t36 would be 1.
> > > > Oh yeah. Does changing it to SETGE fix the miscompile?
> > > Yes it should definitely be SETGE here!
> > OK. I'll switch that to the correct thing. Thanks for the report.
> Yep it does
Sounds good!  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109214



More information about the llvm-commits mailing list