[PATCH] D130191: [RISCV] Teach ComputeNumSignBitsForTargetNode about masked atomic intrinsics

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D130191#3694662 <https://reviews.llvm.org/D130191#3694662>, @asb wrote:

> Yes, my recollection is that the need to handle legalisation of the intrinsics is the main barrier to defining i32 versions on RV64. I'll have another think though.

Yeah, you and Craig are correct here.  I was thinking we had a legal i32 type on rv64 which we don't.  So yeah, ignore me.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9899
+    case Intrinsic::riscv_masked_cmpxchg_i64:
+      // The return value of these intrinsics is produced by an LR_W or
+      // atomicrmw_W instruction and so is sign-extended from 32 bits.
----------------
asb wrote:
> reames wrote:
> > Ok, I finally wrapped my head around all of the machinery here, and this is correct.  However, I want to suggest some changes.
> > 
> > First, the comment:
> > // riscv_masked_atomicrmw_* represents an emulated unaligned atomicrmw operation at the minimum supported atomicrmw width whose result is then sign extended to XLEN.  With +A, the minimum width is 32 for both 64 and 32. 
> > 
> > Second, please add the asserts...
> > assert(XLenVT == 64)
> > assert(getMinCmpXchgSizeInBits() == 32);
> > 
> > 
> > 
> Hi Philip, I've drafted a description that is very similar to yours, but made a few small alterations:
> * Referencing cmpxchg as well at atomicrmw
> * unaligned => narrow.
> 
> I think this is close enough to your suggestion that I'm going to go ahead and commit, but obviously just shout if you think that didn't match your intent and we can resolve in a follow-up patch.
> 
> Thanks.
Your tweak was totally fine.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130191