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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D88979#2323940 <https://reviews.llvm.org/D88979#2323940>, @lebedev.ri wrote:

> In D88979#2323935 <https://reviews.llvm.org/D88979#2323935>, @nikic wrote:
>
>> 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 agree with the general goal -- my point here is that changing isNoopCast() may not be the way to achieve that, because at least some of the current usages do need to include ptr/int casts, and can include them safely (not all usages result in type punning).

In fact, we already have a way to write "isNoopCast() without ptrtoint or inttoptr": `isa<BitCast>`. This didn't quite click before.... You might want to replace `CI->isNoopCast(DL) && LI.getType()->isPtrOrPtrVectorTy() == CI->getDestTy()->isPtrOrPtrVectorTy())` here with just `isa<BitCast>(CI)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88979