[PATCH] D123300: [Clang] Enable opaque pointers by default

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D123300#3467278 <https://reviews.llvm.org/D123300#3467278>, @markus wrote:

> We have run into a slight performance degrading issue with our downstream target. The situation is that we relay on the "consthoist" pass with option "-consthoist-gep=1" to factor out the common parts of GEP expresseions to reduce the number of symbol references.
> For example with the old typed pointers we hade just before that pass
>
>   store i16 100, i16* getelementptr inbounds ([8 x i16], [8 x i16]* @extUeValidatedTps, i16 0, i16 0), align 1
>   store i16 101, i16* getelementptr inbounds ([8 x i16], [8 x i16]* @extUeValidatedTps, i16 0, i16 1), align 1
>   store i16 102, i16* getelementptr inbounds ([8 x i16], [8 x i16]* @extUeValidatedTps, i16 0, i16 2), align 1
>
> resulting in that the first GEP was factored out (with the other two based on it) so in total we had one symbol reference.
>
> Now with opaque pointers we instead have
>
>   store i16 100, ptr @extUeValidatedTps, align 1
>   store i16 101, ptr getelementptr inbounds ([8 x i16], ptr @extUeValidatedTps, i16 0, i16 1), align 1
>   store i16 102, ptr getelementptr inbounds ([8 x i16], ptr @extUeValidatedTps, i16 0, i16 2), align 1
>
> again resulting in that the first GEP is factored out with the remaining GEPs based on it with offset adjusted. The result of this is that we have two symbol references.
>
> So if this pass is to remain functionally equivalent then maybe ConstantHoisting.cpp should be taught to treat `ptr @extUeValidatedTps` the same way as zero indexed GEP of that symbol?

Sounds reasonable in general -- though isn't this a pre-existing problem you'd see if you simply had multiple loads from the same global (without any GEP)? Looking at the current ConstantHoist code, it doesn't seem to really consider the case where a global symbol address can be expensive, it only handles hoisting of large integer values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300