TIFitis added inline comments.
================ Comment at: clang/test/OpenMP/target_data_codegen.cpp:67-68 - // CK1-DAG: call void @__tgt_target_data_end_mapper(ptr @{{.+}}, i64 [[DEV:%[^,]+]], i32 1, ptr [[GEPBP:%.+]], ptr [[GEPP:%.+]], ptr [[SIZE00]], ptr [[MTYPE00]], ptr null, ptr null) - // CK1-DAG: [[DEV]] = sext i32 [[DEVi32:%[^,]+]] to i64 - // CK1-DAG: [[DEVi32]] = load i32, ptr %{{[^,]+}}, // CK1-DAG: [[GEPBP]] = getelementptr inbounds {{.+}}[[BP]] ---------------- Similar to what I discussed for the if clause, the device clause argument need not be recomputed. We can reuse the device argument from the begin mapper for the end mapper as well. ================ Comment at: clang/test/OpenMP/target_data_codegen.cpp:355-356 // Region 00 +// CK2-DAG: [[DEV:%[^,]+]] = sext i32 [[DEVi32:%[^,]+]] to i64 +// CK2-DAG: [[DEVi32]] = load i32, ptr %{{[^,]+}}, // CK2: br i1 %{{[^,]+}}, label %[[IFTHEN:[^,]+]], label %[[IFELSE:[^,]+]] ---------------- When both if clause and device clause are present, the device clause argument is inadvertently brought outside the `IfThen` region as we emit the `llvm:Value` from the `Clang::Expr` for it before making call to createTargetData. I don't think this change would affect any use cases. ================ Comment at: clang/test/OpenMP/target_data_use_device_ptr_codegen.cpp:133-134 } // CK1: [[BEND]]: - // CK1: [[CMP:%.+]] = icmp ne ptr %{{.+}}, null // CK1: br i1 [[CMP]], label %[[BTHEN:.+]], label %[[BELSE:.+]] ---------------- I think it is incorrect to recompute the branch condition here. Consider the following C code: ``` int a = 1; #pragma omp target data map(tofrom : a) if(a > 10) {a = 100;} ``` In this case the if condition is false before executing the region, but becomes true after. If the branch condition is recomputed then it would take the `IfThen` branch here for executing the end_mapper call. This is incorrect and the end_mapper call should not be made partially without a begin _mapper call here. Using the OMPIRBuilder fixes this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150860/new/ https://reviews.llvm.org/D150860 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits