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

[Markus Lavin via Phabricator via cfe-commits]

markus added a comment.

With

  $ ./bin/clang -cc1 -emit-llvm BBI-68673.c

the IR contains the following

  @a2_ldm_i64 = global [4 x [18 x { i64, i64 }]] zeroinitializer, align 16
  %arrayidx = getelementptr inbounds [4 x [18 x { i64, i64 }]], ptr 
@a2_ldm_i64, i64 0, i64 %idxprom
  %.real = load volatile i64, ptr getelementptr inbounds ([18 x { i64, i64 }], 
ptr @a2_ldm_i64, i64 0, i64 1), align 16

but I would expect both GEPs to have the same first argument type (as the 
global symbol)

If I on the other hand compile with

  $ ./bin/clang -cc1 -emit-llvm -no-opaque-pointers BBI-68673.c

I instead get

  @a2_ldm_i64 = global [4 x [18 x { i64, i64 }]] zeroinitializer, align 16
  %arrayidx = getelementptr inbounds [4 x [18 x { i64, i64 }]], [4 x [18 x { 
i64, i64 }]]* @a2_ldm_i64, i64 0, i64 %idxprom
  %.real = load volatile i64, i64* getelementptr inbounds ([4 x [18 x { i64, 
i64 }]], [4 x [18 x { i64, i64 }]]* @a2_ldm_i64, i64 0, i64 0, i64 1, i32 0), 
align 16

which looks more consistent.

Any idea what is going on here?

F22817439: BBI-68673.c 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Markus Lavin via Phabricator via cfe-commits]

markus added a comment.

In D123300#3459023 , @nikic wrote:

> @markus Without tracing through it in detail, I'd guess that without opaque 
> pointers this creates two getelementptr constant expressions that get folded 
> together. With opaque pointers, the first one (which would be a zero-index 
> GEP) is omitted, and only the second one is left. ConstantFolding will later 
> canonicalize the GEP source type, but this only happens when InstCombine 
> runs, while you're looking at unoptimized IR.

Yes, that appears to have been the case. Thanks for explaining.

> Are you encountering some particular issue relating to this? For well-written 
> optimizations, the exact GEP representation shouldn't matter either way.

One of our target specific passes ran into problems with this as it made some 
assumptions that no longer hold with opaque pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Markus Lavin via Phabricator via cfe-commits]

markus added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Markus Lavin via Phabricator via cfe-commits]

markus added a comment.

In D123300#3467309 , @nikic wrote:

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

Indeed, multiple (GEP-less) loads from the same symbol gives the same problem. 
At least if they are spread out across the CFG, otherwise isel deals with it. 
Not sure what is the right way to deal with this. Maybe we will try making some 
modifications to ConstantHoist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits