[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:180
+/// additional pointers to try. Returns the first non-null return value from
+/// Func, or nullptr if the type can't be introspected further.
 static Constant *
----------------
aeubanks wrote:
> ConstantLoad?
I thought renaming would help clarify what the callback is expected to do. Here in some sense it behaves kind of like a constexpr load would, returning a constant if it can evaluate a load from a constant pointer, and nullptr if it cannot.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105838