[PATCH] D113566: [fir] Add fir.box_isarray, fir.box_isptr and fir.box_isalloc conversion.

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Makes sense, left a few minor comments/nits.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:33
 
+/// `fir.box` attribute values as defined in flang/ISO_Fortran_binding.h.
+static constexpr unsigned kAttrPointer = CFI_attribute_pointer;
----------------
i.e. `CFI_attribute_t`?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:34-35
+/// `fir.box` attribute values as defined in flang/ISO_Fortran_binding.h.
+static constexpr unsigned kAttrPointer = CFI_attribute_pointer;
+static constexpr unsigned kAttrAllocatable = CFI_attribute_allocatable;
+
----------------
clementval wrote:
> clementval wrote:
> > awarzynski wrote:
> > > Why not use the macro directly?
> > `fir.box` is an abstraction of a descriptor also we can get a compile time error in case the macro is not a uint. 
> also it makes it homogenous with the `fir.box` field indexing added previously so the code is maybe easier to understand. 
> we can get a compile time error in case the macro is not a uint

Any idea why wouldn't the original value be define as `uint`? That would make it easier for you here.

> also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.

Agreed, this does help a lot!

I think that you are doing a great job and a massive favour to all of us here. I'm just thinking that perhaps stuff  from "flang/ISO_Fortran_binding.h" should be wrapped in an enum class or something similar? Basically, sth to facilitate code re-use a bit better. WDYT? 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:125-127
+  // Load the attribute from the `fir.box` and perform a logical `and` with the
+  // mask value. The value returned is the result the integer comparison (NE)
+  // between the logical `and` result and 0.
----------------
DOXYGENME

IMHO, this comment is a bit unclear. How about:
```
Creates a sequence of instructions to load the box attribute from \p box and to check it against  \p maskValue. The final check is implemented as `(attribute && maskValue) != 0`.
```
Or something similar?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:270-271
+/// boxed is an array.
+/// It uses the same sequences as `fir.box_rank` to extract the rank value and
+/// compare it to 0.
+struct BoxIsArrayOpConversion : public FIROpConversion<fir::BoxIsArrayOp> {
----------------
[nit] This is the case just now, but might not be in the future. Also, the two implementations are independent (i.e. there's no code re-use). So I would delete this. Only the first sentence is key.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:769
+
+// Test `fir.box_isarray` conversion.
+
----------------
[nit] Perhaps add that it will load the rank and comparer it against 0?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:788
+
+// Test `fir.box_isalloc` conversion.
+
----------------
[nit] Perhaps add that it will load the attribute and check it against the mask corresponding to `CFI_attribute_allocatable`?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:799
+// CHECK:         %[[ATTRPOS:.*]] = llvm.mlir.constant(5 : i32) : i32
+// CHECK:         %[[GEP:.*]] = llvm.getelementptr %[[ARG0]][%[[C0]], %[[ATTRPOS]]] : (!llvm.ptr<struct<(ptr<f64>, i64, i32, i8, i8, i8, i8)>>, i32, i32) -> !llvm.ptr<i32>
+// CHECK:         %[[ATTR:.*]] = llvm.load %[[GEP]] : !llvm.ptr<i32>
----------------
Make this platform agnostic?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:809
+
+// Test `fir.box_isptr` conversion.
+
----------------
[nit] Perhaps add that it will load the attribute and check it against the mask corresponding to CFI_attribute_pointer?




================
Comment at: flang/test/Fir/convert-to-llvm.fir:820
+// CHECK:         %[[ATTRPOS:.*]] = llvm.mlir.constant(5 : i32) : i32
+// CHECK:         %[[GEP:.*]] = llvm.getelementptr %[[ARG0]][%[[C0]], %[[ATTRPOS]]] : (!llvm.ptr<struct<(ptr<f64>, i64, i32, i8, i8, i8, i8)>>, i32, i32) -> !llvm.ptr<i32>
+// CHECK:         %[[ATTR:.*]] = llvm.load %[[GEP]] : !llvm.ptr<i32>
----------------
Make this platform agnostic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113566