[PATCH] D48131: [RISCV] Implement codegen for cmpxchg on RV32I

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1705
+  Function *MaskedCmpXchg = Intrinsic::getDeclaration(
+      CI->getModule(), Intrinsic::riscv_masked_cmpxchg_i32, Tys);
+  return Builder.CreateCall(MaskedCmpXchg,
----------------
asb wrote:
> rogfer01 wrote:
> > @asb, I have a minor question here:
> > 
> > At some point we will want to implement this for rv64a. I presume that for rv64a we still want to have `setMinCmpXchgSizeInBits(32)` so the `i32` and the `i64` cases are handled in `RISCVExpandPseudoInsts.cpp` as non-masked lr/sc loops. Currently `AtomicExpand.cpp` will choose to use the smaller size supported (`i32`) and the intrinsic here will be using `i32` values. This will require some form of legalisation in 64-bit, won't it? Will we end custom legalising this intrinsic call for the 64-bit case to a `riscv_masked_cmpxchg_i64`? Or perhaps there is a neater way to achieve this?
> > 
> > Thanks!
> Hi Roger. If i64 is the only legal type, I don't see a way around having an i64 form of the intrinsic. You then need to promote the operands to i64.
> 
> So you want to do something like
> 
> 
> ```
> unsigned XLen = Subtarget.getXLen();
> Value *Ordering = Builder.getIntN(XLen, static_cast<uint64_t>(Ord));
> ```
> 
> and use Builder.CreateSExt on the other operands.
Hi Alex,

If we have an i64 intrinsic, do we expect it to return i64 too? I'd say yes but then `AtomicExpand.cpp` (line 964), which has been working on the minimum supported CAS type of `i32` will attempt to build a `shr` mixing an `i64` and `i32` and the builder complains.

Maybe I have give up the idea of having i32 as the minimum cas type in rv64a and set it to i64, and then devise a way to use a non-masked LL/SC loop later.

Anyways I don't want to bother you too much with rv64 at this point.

Thanks a lot for the prompt answer.


https://reviews.llvm.org/D48131