[PATCH] D97133: [SelectionDAG][RISCV] Teach ComputeNumSignBits to handle SREM.

Simon Pilgrim via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 21 11:04:05 PST 2021


RKSimon accepted this revision.
RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3877
+    // at least as many sign bits as the left hand side.
+    return ComputeNumSignBits(Op.getOperand(0), DemandedElts, Depth + 1);
   case ISD::TRUNCATE: {
----------------
craig.topper wrote:
> craig.topper wrote:
> > luismarques wrote:
> > > RKSimon wrote:
> > > > This is a lot more relaxed than ValueTracking ?
> > > Interesting! ValueTracking seems to abort on `!Denominator->isStrictlyPositive()`. When I reviewed the logic of this code at first I thought there would be cases where it would break, but when I thought about it more deeply it all seemed fine. Now I'm curious about what may explain the divergence. Maybe the implementation in ValueTracking was considering modulus instead of remainder?
> > The implementation in value tracking only handles constant right hand side. Looking at the history, its original implementation was incorrect and maybe thought the result was always positive at least from the the original comment.
> > 
> > I didn't handle constant here because I figured most targets would expand an srem by constant using BuildSDIV in TargetLowering.
> It might make sense to update ValueTracking to hanldle the non-constant case. Some experiments I did from C seemed to get helped by CorrelatedValuePropagation. For example a shl after an srem with extra sign bits can pick up an nsw flag through CVP. This allows InstSimplify/InstCombine to late remover a shl+ashr sign extend pair due to the nsw.
That sounds like a great followup but I'm happy for this to go ahead independently. Thanks for looking into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97133



More information about the llvm-commits mailing list