[PATCH] D123919: [mlir][OpenMP] omp.task translation to LLVM IR

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:691
+  }
+  auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codegenIP) {
+    builder.restoreIP(codegenIP);
----------------
peixin wrote:
> Is allocaIP used in this?
It is not used in this instance, but it is required in `OpenMPIRBuilder` for the most general case and so this is required.


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:2146
+  // CHECK: %[[task_data:.+]] = call i8* @__kmpc_omp_task_alloc
+  // CHECK-SAME: (%{{.+}}, i32 %[[omp_global_thread_num]], i32 1, i64 16, i64 0,
+  // CHECK-SAME: i32 (i32, i8*)* bitcast (i32 (i32, { i32, i32, i32* }*)*
----------------
Meinersbur wrote:
> shraiysh wrote:
> > kiranchandramohan wrote:
> > > Why is the task size 16 and not 12 (for the three liveins)?
> > `x` and `y` are 4-byte each, and the pointer `zaddr` is of 8 bytes (64-bit). So, the total is 16.
> A pointer is 8 bytes only on 64-bit platforms. Does this mean that this test will fail on platforms that have pointers of different sizes? I also don't see the target triple fixed by mlir-translate, so will this test fail on non-64 hosts?
It could. I don't know who should set the target-triple/data-layout. Should it be a command line option for mlir-translate? A similar test is there in OpenMP IRBuilder [[ https://github.com/llvm/llvm-project/blob/facbfb121a5c0a78b3cc4ecc41b15dcef8d9c6a5/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp#L4746 | here ]], which could cause issues (it hasn't caused any so far) and I should probably rectify that too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123919