[PATCH] D130191: [RISCV] Teach ComputeNumSignBitsForTargetNode about Intrinsic::riscv_masked_cmpxchg_i64

Tue Aug 2 02:56:11 PDT 2022

asb marked an inline comment as done.
asb added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9682
+      // ANDing the result with a mask. Therefore, the sign bits are the
+      // minimum of 33 (from the LR_W) and the number of sign bits in the mask
+      // operand.
----------------
craig.topper wrote:
> asb wrote:
> > reames wrote:
> > > I don't follow this comment.  Is there some documentation you can point to for what this intrinsic does?  Or a good pointer in code to understand it?
> > > 
> > > If I'm gathering this correctly, the and-by-mask is to handle the zext of the type less than XLEN?  Not following where the sign bit below that is coming from.  
> > The intrinsics are underdocumented - I'll loop back round and fix that. For general context on our atomics lowering approach, see [here](https://lists.llvm.org/pipermail/llvm-dev/2018-June/123993.html).
> > 
> > As for this specific code comment: it may be because it's the end of the day, but I started trying to better explain and confused myself. So it may be I have a mistake, the code comment needs improving, or both. Just leaving this comment now, as it may be it's best for you to hold off on taking a proper look until I've had a chance to re-review.
> Isn't the output of the intrinsic only the result of the LR_W? There are ANDs in the expansion, but they are to the scratch register aren't they?
I've improved docs for intrinsics in [85c6fab](https://reviews.llvm.org/rG85c6fab8d317).

You're both completely right that I made a logic error here initially - the intrinsic doesn't mask its return value at all, so the number of sign bits is always just 33.

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

https://reviews.llvm.org/D130191