[PATCH] D134308: AtomicExpand: Use llvm.ptrmask instead of ptrtoint

Thu Sep 22 08:00:11 PDT 2022

jyknight added a comment.

In D134308#3804439 <https://reviews.llvm.org/D134308#3804439>, @arsenm wrote:

> In D134308#3804313 <https://reviews.llvm.org/D134308#3804313>, @jyknight wrote:
>
>> In D134308#3804305 <https://reviews.llvm.org/D134308#3804305>, @arsenm wrote:
>>
>>> In D134308#3804295 <https://reviews.llvm.org/D134308#3804295>, @jyknight wrote:
>>>
>>>> Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?
>>>
>>> Moving towards the goal of never having compiler introduced inttoptr
>>
>> That was sort of my guess -- but, then, this change already DOES fix (this part of) the problem, right? We have a ptrtoint remaining, but not an inttoptr -- that should be fine?
>
> It half fixes it. It's still introducing an inttoptr in order to get the low bits. We could introduce a second ptrmask with the inverted mask, but we still need to inttoptr in order to shift that into the Inv_Mask position

OK, now I'm confused again...

My understanding:.

- converting from integer to pointer ("inttoptr") is undesirable to introduce in transformations. (Such instruciton may be present in the original input, but shouldn't be added if it was not.)
- converting from pointer to integer ("ptrtoint") is OK.

In the current AtomicExpandPass.cpp, we have 3 mentions of CreateIntToPtr:

- convertAtomicXchgToIntegerType
- convertCmpXchgToIntegerType
- createMaskInstrs

The first two are about pointer-valued atomicrmw and cmpxchg operands, which are unrelated to this change, so we'll ignore that. The third is removed by this patch.

So, ISTM this patch does entirely fix the problem it sets out to fix -- it has removed the only CreateIntToPtr from the address computation. There remains a CreatePtrToInt to extract the value shift/mask -- but that's not a problem. Right?

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

https://reviews.llvm.org/D134308