[PATCH] D144445: [ConstantFold][InstSimplify] folding load for constant global all-element-equal arrays and structs

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Please take a look at IR produced by rustc: https://godbolt.org/z/c88Wadrbn

Note that rustc always represents constant globals in raw representation: The two lookup tables here are `<{ [2 x i8] c"\01\01" }>` and `<{ [4 x i8] c"\01\00\01\00" }>`. Especially note that the latter is not `[i16 1, i16 1]` or similar, it's all represented as bytes.

Your current code will not be able to handle such cases, because it depends on structural reasoning. The way to support this would be to use offset-based reasoning: The GEP instruction has a form like `@g + 4 * x`, so one could call ConstantFoldLoadFromConst() with offset 0, then 4, then 8, etc and check that the result is always the same. This way it is independent of the representation (but of course, less compile-time efficient).

In D144445#4140857 <https://reviews.llvm.org/D144445#4140857>, @khei4 wrote:

> proof: https://alive2.llvm.org/ce/z/_yEafa

This is a bit subtle, because your input IR has an implicit `align 4` assumption on the load. There is a miscompile if you remove it: https://alive2.llvm.org/ce/z/H9m9uN The alignment effectively forces that `%idx` is a multiple of 4, but generally we cannot assume this.

> Might be a little compile-time regression on some benchmarks

The link here is broken, but I assume you meant this one: http://llvm-compile-time-tracker.com/compare.php?from=38ed416693c29761e8a934144102def74b4b47b3&to=cddf9d1ed0bde41b392352bf2391d065e2d27c35&stat=instructions:u Note that the kimwitu++ regression is due to an increase in code size: http://llvm-compile-time-tracker.com/compare.php?from=38ed416693c29761e8a934144102def74b4b47b3&to=cddf9d1ed0bde41b392352bf2391d065e2d27c35&stat=size-text This is pretty normal -- if code size changes, there is usually corresponding compile-time change.



================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:770
 
+Constant *llvm::ConstantFoldLoadFromAllEqAggregate(Constant *C, Type *Ty) {
+
----------------
junparser wrote:
> khei4 wrote:
> > junparser wrote:
> > > What you need maybe something like Constant::getSplatValue with proper handle with constant struct and array
> > @junparser
> > Thank you for the review! 
> > Yeah, we can say so, but `Splat` seems like from [[ https://lists.llvm.org/pipermail/llvm-dev/2017-May/112758.html | conventions around vector type ]] 
> > , so if implemented as a method, it might be better to separate them.
> > As you see, architecturally this element's equality checking might be general enough to implement as a Constant method. Is it better?
> Since this is complement for ConstantFoldLoadFromUniformValue, so it is better to do inside ConstantFoldLoadFromUniformValue.
This should not be part of ConstantFoldLoadFromUniformValue.


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

https://reviews.llvm.org/D144445