[PATCH] D105838: [GlobalOpt] Fix a miscompile when evaluating struct initializers.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Evaluator.cpp:374
+            // The conversion is illegal if the store is wider than the
+            // proposed bitcast load, since that would drop stores to other
+            // struct elements when `evaluateBitcastFromPtr` attempts to look
----------------
jroelofs wrote:
> efriedma wrote:
> > "proposed bitcast load" doesn't make sense.  Do you mean to say "The conversion is illegal if the stored type is narrower than the type of the memory location"?
> > 
> > The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs.  But you don't need to do that rewrite here.
> For context, I meant "proposed bitcast load" in the sense that `evaluateBitcastFromPtr` making queries to its callback first for the whole struct, then for that struct's first element, etc. is a "proposal" with nullptr return being a rejection, and the evaluation of the callback itself being a sort of constexpr-through-a-bitcast. Agreed that the wording is strange; I'll reword.
> 
> > The code would probably be easier to understand...
> 
> I was thinking the same, but wanted to keep the fix simple.
> The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs. But you don't need to do that rewrite here.

I'm beginning to think a representational change is *necessary* in order to fix all the issues here. Consider this, for example:

https://llvm.godbolt.org/z/4jhn9PGdW


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105838