[PATCH] D88979: [InstCombine] combineLoadToOperationType(): don't fold int<->ptr cast into load

Sun Oct 11 10:07:57 PDT 2020

lebedev.ri added a comment.

@nlopes @nikic thank you for the review!

In D88979#2323948 <https://reviews.llvm.org/D88979#2323948>, @nlopes wrote:

> In D88979#2323940 <https://reviews.llvm.org/D88979#2323940>, @lebedev.ri wrote:
>
>> In D88979#2323935 <https://reviews.llvm.org/D88979#2323935>, @nikic wrote:
>>
>>> LGTM
>>
>> @nlopes does this look good to you?
>>
>>> Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.
>>
>> I think i don't agree with you there.
>> I agree with @nlopes, the end goal will be to basically disallow fusing of `inttoptr`/`ptrtoint` into loads, 
>> disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
>> And all that eventually boils down to updating `CastInst::isNoopCast()`/`CastInst::isEliminableCastPair()`.
>
> I'm ok with this change, and I agree it's a good step forwards. Thanks!
> My only concern was that it seems this code will need further changes going forward (maybe even revert this change?) and I wouldn't want us to forget to revisit this code if/when needed.

Once those methods are fixed, sure, the uses will need dead code cleanup.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88979