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

[Dominik Adamski via Phabricator via cfe-commits]

domada added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2976
 
+  const int DefaultAlignment = 16;
+
----------------
jdoerfert wrote:
> 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.
>> 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 rely on Clang functionality and I assume that the Clang always sets the size(default or specified by the user). That's why I added additional `assert` to applySimd function  to be sure that Clang always sets the alignment value.

Currently Clang does not use the actual type to calculate the default simd alignment: https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 . IMO it's better to rely on Clang and it's functionality which is widely tested.


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

https://reviews.llvm.org/D133578