[PATCH] D88789: [InstCombine] Revert rL226781 "Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available." (PR47592)

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D88789#2310967 <https://reviews.llvm.org/D88789#2310967>, @efriedma wrote:

> In D88789#2310606 <https://reviews.llvm.org/D88789#2310606>, @chandlerc wrote:
>
>> FWIW, I still very much feel that this is the correct canonicalization, and that downstream problems *must* be fixed downstream. Avoiding this canonicalization doesn't actually fix them, it just makes us less *aware* of the problems that still fundamentally exist. =[
>
> I'd agree if we excluded all pointers from canonicalization.  But the semantics of inttoptr and inttoptr-equivalent memory operations are weird; in general, I'm not sure we can recover the original semantics of the code if we throw away the pointer-ness of pointer load/store operations.
>
> To address the issue at hand, I think changing the isNonIntegralPointerType() check to just isPtrOrPtrVectorTy() would be enough.  I think that might make sense?

Keeping loads and stores of pointers as pointers to the extent possible doesn't seem like a bad idea, but I'm worried people will feel like this gives a *semantic* guarantee that isn't really there. Fundamentally, LLVM still doesn't currently have typed memory. All of the optimizer is built upon this assumption.

Anyways, while it doesn't seem intrinsically bad to preserve pointer types as much as possible, I feel like the underlying problem should be addressed in a more fundamental way -- that this change will just shift the problem to more complex cases where the frontend happens to use a memcpy or something similar. I wonder if revisiting D75505 <https://reviews.llvm.org/D75505> makes somewhat more sense, although clearly it would need some different approach to address the compile time issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88789