[PATCH] D140347: TargetLowering: Teach DemandedBits about VSCALE

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:1136
+      return false;
+    APInt VScaleResultUpperbound(64, *MaxVScale);
+    VScaleResultUpperbound *= Op.getConstantOperandAPInt(0).sextOrTrunc(64);
----------------
craig.topper wrote:
> I'm not sure that hardcoding 64 here is the right fix. I think what we need to check is that the multiply doesn't overflow.
@craig.topper I absolutely agree with you.  I call this out in the commit message, that I'm just assuming that this is wide enough.  This can potentially overflow still.  Is there a good way to ensure that we have the proper width?


================
Comment at: llvm/test/CodeGen/RISCV/vscale-demanded-bits.ll:3
+
+; CHECK: vse8.v v8, (a5), v0.t
+; CHECK: vadd.vx v8, v8, a3
----------------
jrtc27 wrote:
> Use UTC, really
What do you mean by UTC?


================
Comment at: llvm/test/CodeGen/RISCV/vscale-demanded-bits.ll:8
+
+define dso_local void @_Z4FillPhi(ptr nocapture noundef writeonly %buffer, i32 noundef signext %n) local_unnamed_addr {
+entry:
----------------
jrtc27 wrote:
> I tend to prefer tests that don't have mangled C++ names in them
Sure, I can rename it, don't really think that it matters though.


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

https://reviews.llvm.org/D140347