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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/requested changes.

As an optional follow up, I find myself wondering if the 64 bit versions should actually have a 64 bit return type.  Having them instead have i32 operands and return types - to model the emulated access actually being done - and then explicitly sign extended afterwards would get the same effect here with less code and less confusion.



================
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.
----------------
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);





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

https://reviews.llvm.org/D130191