[PATCH] D113469: [fir] Add fir.convert op conversion from FIR to LLVM IR

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Makes sense, I've left a few small suggestions. Thanks for working on this!



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:138
+    };
+    if (fir::isa_complex(convert.value().getType()) &&
+        fir::isa_complex(convert.res().getType())) {
----------------
Could you add a comment, e.g. "// Complex to complex conversion"?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:161
+    }
+    if (isFloatingPointTy(fromTy)) {
+      if (isFloatingPointTy(toTy)) {
----------------
Could you add a comment? e.g. "Conversion from fp values"


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:173
+      }
+    } else if (fromTy.isa<mlir::IntegerType>()) {
+      if (toTy.isa<mlir::IntegerType>()) {
----------------
> Don’t use else after a return
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Could you add a comment, e.g. "Conversion from integer types"?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:193
+      }
+    } else if (fromTy.isa<mlir::LLVM::LLVMPointerType>()) {
+      if (toTy.isa<mlir::IntegerType>()) {
----------------
> Don’t use else after a return
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Could you add a comment, e.g. "// Conversion from pointer types"?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:575
+// CHECK:         %{{.*}} = llvm.trunc %[[ARG0]] : i32 to i16
+// CHECK:         %{{.*}} = llvm.sext %[[ARG0]] : i32 to i64
+// CHECK:         %{{.*}} = llvm.inttoptr %{{.*}} : i64 to !llvm.ptr<i64>
----------------
Missing:
```
// CHECK-NOT:         %{{.*}} = llvm.trunc %[[ARG0]] : i32 to i32
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113469