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

Wed Sep 8 03:58:29 PDT 2021

dmgreen 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:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109214