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

Mon Jun 6 14:08:57 PDT 2022

Meinersbur added inline comments.

================
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* }*)*
----------------
shraiysh wrote:
> 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. 
The OpenMPIRBuilder tests use an empty TargetTriple (`""`) which represents an 64-bit architecture with byte-aligment. What does `mlir-translate` do? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123919