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

Sun Jul 24 15:56:50 PDT 2022

craig.topper 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.
----------------
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?

================
Comment at: llvm/test/CodeGen/RISCV/atomic-cmpxchg-branch-on-result.ll:132
 ; RV64IA-NEXT:    # in Loop: Header=BB2_1 Depth=1
 ; RV64IA-NEXT:    and a4, a4, a0
-; RV64IA-NEXT:    sext.w a4, a4
----------------
I don't think this AND is part of the intrinsic. It's a separate operation in the IR and DAG. The output of the intrinsic is a4, the lr.w.aqrl result.

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

https://reviews.llvm.org/D130191