[PATCH] D133578: [OpenMP][OMPIRBuilder] Add generation of SIMD align assumptions to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2976
 
+  const int DefaultAlignment = 16;
+
----------------
domada wrote:
> jdoerfert wrote:
> > domada wrote:
> > > jdoerfert wrote:
> > > > This doesn't work. Use the data layout for any default values please.
> > > I have used pointer ABI alignment as the default value. Is it ok?
> > > Clang has separate method to calculate OpenMP default alignment defined in TargetInfo class ( [[ https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 |  link ]])
> > > Unfortunately, there is no simple replacement of this function in OMPIRBuilder or in Flang driver. There are some requests to expose TargetInfo data in Clang-independent manner: [[ https://discourse.llvm.org/t/rfc-targetinfo-library/64342 | RFC1 ]], [[https://discourse.llvm.org/t/complex-to-libm-conversion-abi-issues/65131 |RFC2]] , but this issue is not solved.
> > Let's step back:
> > 1) User tells us the alignment via `aligned(X:64)`. This should populate the map in Clang, I would assume.
> > 2) User tells us the alignment "implicitly" via `align(Y)`. Again, clang should populate the map with the default/preferred alignment of the type. You can use the function you linked, or use the data layout API to get the preferred alignment.
> > 3) User tells us nothing. We should pick the default or, don't annotate anything. If we go with defaults, you need to look up the default for the address space but I don't know why we would annotate anything not provided by the user.
> > 
> > 
> Ad 1) Agree
> Ad 2) OK, I will add assertion to ensure that the map is correctly populated with the default/preferred alignment if the user specifies only: `align(Y)`.
> 
> I was thinking that OMPIRBuilder should pick the default value if the user specifies `align(Y)`. Thanks for clarification. 
> 
> Ad 3)  I assume that if the user tells us nothing we should not annotate anything. I will expect empty map and no generated annotations. Currently we do not generate any annotations if there is no align clause.
> I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.

That works with me, but you need the actual type. So the Value is not sufficient, you need `&X and double` to ask DataLayout for the preferred type. For now you can use the clang functionality or pass the type and use DL.

> I assume that if the user tells us nothing we should not annotate anything. 

Agreed.


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

https://reviews.llvm.org/D133578