[Openmp-commits] [PATCH] D149368: [MLIR][OpenMP] Initial Lowering of Declare Target for Data

[Andrew Gozillon via Phabricator via Openmp-commits]

agozillon added a comment.

I think I've covered all the review points you made in the last update, please let me know if I'm missing anything though!



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1668
+    auto mapFlag = llvm::omp::OpenMPOffloadMappingFlags(
+        mapTypes[index].dyn_cast<IntegerAttr>().getUInt());
 
----------------
skatrak wrote:
> If we can assert at this point that `mapTypes` can only contain `IntegerAttr` at this point, it's better to do `mapTypes[index].cast<IntegerAttr>`. Otherwise, we should check the result of the `dyn_cast` before calling `getUInt` on it to avoid a crash.
It can only contain an IntegerAttr, a UInt to be precise in this case, otherwise there'd be complaints somewhere else in the compiler about mismatched types! So in this case I've swapped it to a cast.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149368