[PATCH] D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:571
+      // Note that if load is atomic, we might not be able to change load type.
+      if (LI.isAtomic() && !isSupportedAtomicType(DestTy))
+        return nullptr;
----------------
jfb wrote:
> lebedev.ri wrote:
> > jfb wrote:
> > > lebedev.ri wrote:
> > > > jfb wrote:
> > > > > This seems like a pre-existing bug: you should bail on volatile as well. For example, if user code does a volatile load of int32 and then casts to float, we should probably try to emit an integer load (even if that's not guaranteed by the language).
> > > > I can do that, but could you point me to the appropriate langref wording?
> > > I don't think we're super clear on what we guarantee, but knowing people who use volatile: when they say "load an integer and cast it to a float" they expect to do an integer, followed by an integer->float register transfer. That's a reasonable expectation IMO.
> > People also expect UB to be ignored by compiler.
> > Unless it's spelled out in langref i don't believe i know why this should be changed.
> That's a user-hostile approach to compiler optimizations. It's not a useful thing to optimize, and developers don't have a way to express this, so they'd have to turn to assembly. We could simply be nice and do what's trivial for us to do.
That is not what i'm saying :)

You have either pointed out a miscompile, or not. If it's a miscompile,
then someone knowledgeable (you) should document it [in langref]
for future reference, to avoid having this disscussion in the future
and simplify the life of whoever would want to be aware of it later.

I don't care either way, i just don't want to have such undocumented
limitation/guaranteed spelled out in the code and not in documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75505