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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
