fix optional wait wrongly treated as false #78149
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a optional bool in passed as optional, isStaticallyPresent() will actually return true. By changing wait from asValue to asAddr and add some check can solve this.
|
When testing with program bug
call inner("sleep 2 && echo hi")
print *, "Direct printout line"
contains
subroutine inner(cmd, wait, exitstatarg, cmdstatarg, cmdmsgarg)
character(20) :: cmd
logical, optional :: wait
integer, optional :: exitstatarg, cmdstatarg
character(128), optional :: cmdmsgarg
integer :: localvar
call execute_command_line(cmd, wait, exitstatarg, localvar, cmdmsgarg)
end subroutine
endThe console print out would be: And when set wait to false, |
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Yi Wu (yi-wu-arm) ChangesWhen an optional bool in passed as optional, Full diff: https://github.com/llvm/llvm-project/pull/78149.diff 3 Files Affected:
diff --git a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
index ac7d4fbe23e6738..f958e428f9bebf5 100644
--- a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
+++ b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
@@ -217,7 +217,7 @@ static constexpr IntrinsicHandler handlers[]{
{"execute_command_line",
&I::genExecuteCommandLine,
{{{"command", asBox},
- {"wait", asValue, handleDynamicOptional},
+ {"wait", asAddr, handleDynamicOptional},
{"exitstat", asBox, handleDynamicOptional},
{"cmdstat", asBox, handleDynamicOptional},
{"cmdmsg", asBox, handleDynamicOptional}}},
@@ -2913,7 +2913,8 @@ IntrinsicLibrary::genEoshift(mlir::Type resultType,
// EXECUTE_COMMAND_LINE
void IntrinsicLibrary::genExecuteCommandLine(
llvm::ArrayRef<fir::ExtendedValue> args) {
- assert(args.size() == 5);
+ // Optional KIND argument.
+ assert(args.size() >= 1);
mlir::Value command = fir::getBase(args[0]);
const fir::ExtendedValue &wait = args[1];
const fir::ExtendedValue &exitstat = args[2];
@@ -2925,9 +2926,30 @@ void IntrinsicLibrary::genExecuteCommandLine(
mlir::Type boxNoneTy = fir::BoxType::get(builder.getNoneType());
- mlir::Value waitBool = isStaticallyPresent(wait)
- ? fir::getBase(wait)
- : builder.createBool(loc, true);
+ mlir::Value waitBool;
+ if (isStaticallyAbsent(wait)) {
+ waitBool = builder.createBool(loc, true);
+ } else {
+ mlir::Type i1Ty = builder.getI1Type();
+ mlir::Value waitAddr = fir::getBase(wait);
+ mlir::Value waitIsPresentAtRuntime =
+ builder.genIsNotNullAddr(loc, waitAddr);
+ waitBool = builder
+ .genIfOp(loc, {i1Ty}, waitIsPresentAtRuntime,
+ /*withElseRegion=*/true)
+ .genThen([&]() {
+ auto waitLoad = builder.create<fir::LoadOp>(loc, waitAddr);
+ mlir::Value cast =
+ builder.createConvert(loc, i1Ty, waitLoad);
+ builder.create<fir::ResultOp>(loc, cast);
+ })
+ .genElse([&]() {
+ mlir::Value trueVal = builder.createBool(loc, true);
+ builder.create<fir::ResultOp>(loc, trueVal);
+ })
+ .getResults()[0];
+ }
+
mlir::Value exitstatBox =
isStaticallyPresent(exitstat)
? fir::getBase(exitstat)
diff --git a/flang/test/Lower/Intrinsics/execute_command_line-optional.f90 b/flang/test/Lower/Intrinsics/execute_command_line-optional.f90
index e51c0e5fca3004e..57fbacc51cc84fe 100644
--- a/flang/test/Lower/Intrinsics/execute_command_line-optional.f90
+++ b/flang/test/Lower/Intrinsics/execute_command_line-optional.f90
@@ -21,18 +21,10 @@ subroutine all_args_optional(command, isWait, exitVal, cmdVal, msg)
! CHECK-NEXT: %[[cmdmsgUnbox:.*]]:2 = fir.unboxchar %[[cmdmsgArg]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK-NEXT: %[[cmdmsgDeclare:.*]] = fir.declare %[[cmdmsgUnbox]]#0 typeparams %[[cmdmsgUnbox]]#1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "_QFall_args_optionalEmsg"} : (!fir.ref<!fir.char<1,?>>, index) -> !fir.ref<!fir.char<1,?>>
! CHECK-NEXT: %[[cmdmsgBoxTemp:.*]] = fir.emboxchar %[[cmdmsgDeclare]], %[[cmdmsgUnbox]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.boxchar<1>
-! CHECK-NEXT: %[[waitIsPresent:.*]] = fir.is_present %[[waitDeclare]] : (!fir.ref<!fir.logical<4>>) -> i1
! CHECK-NEXT: %[[exitstatIsPresent:.*]] = fir.is_present %[[exitstatDeclare]] : (!fir.ref<i32>) -> i1
! CHECK-NEXT: %[[cmdstatIsPresent:.*]] = fir.is_present %[[cmdstatDeclare]] : (!fir.ref<i32>) -> i1
! CHECK-NEXT: %[[cmdmsgIsPresent:.*]] = fir.is_present %[[cmdmsgBoxTemp]] : (!fir.boxchar<1>) -> i1
! CHECK-NEXT: %[[commandBox:.*]] = fir.embox %[[commandDeclare]] typeparams %[[commandUnbox]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.box<!fir.char<1,?>>
-! CHECK-NEXT: %[[waitLoaded:.*]] = fir.if %[[waitIsPresent]] -> (!fir.logical<4>) {
-! CHECK-NEXT: %[[VAL_31:.*]] = fir.load %[[waitDeclare]] : !fir.ref<!fir.logical<4>>
-! CHECK-NEXT: fir.result %[[VAL_31]] : !fir.logical<4>
-! CHECK-NEXT: } else {
-! CHECK-NEXT: %[[VAL_31:.*]] = fir.convert %false : (i1) -> !fir.logical<4>
-! CHECK-NEXT: fir.result %[[VAL_31]] : !fir.logical<4>
-! CHECK-NEXT: }
! CHECK-NEXT: %[[exitstatArgBox:.*]] = fir.embox %[[exitstatDeclare]] : (!fir.ref<i32>) -> !fir.box<i32>
! CHECK-NEXT: %[[absentBoxi32:.*]] = fir.absent !fir.box<i32>
! CHECK-NEXT: %[[exitstatBox:.*]] = arith.select %[[exitstatIsPresent]], %[[exitstatArgBox]], %[[absentBoxi32]] : !fir.box<i32>
@@ -41,8 +33,16 @@ subroutine all_args_optional(command, isWait, exitVal, cmdVal, msg)
! CHECK-NEXT: %[[cmdmsgArgBox:.*]] = fir.embox %[[cmdmsgDeclare]] typeparams %[[cmdmsgUnbox]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.box<!fir.char<1,?>>
! CHECK-NEXT: %[[absentBox:.*]] = fir.absent !fir.box<!fir.char<1,?>>
! CHECK-NEXT: %[[cmdmsgBox:.*]] = arith.select %[[cmdmsgIsPresent]], %[[cmdmsgArgBox]], %[[absentBox]] : !fir.box<!fir.char<1,?>>
+! CHECK-NEXT: %[[waitCast:.*]] = fir.convert %[[waitDeclare]] : (!fir.ref<!fir.logical<4>>) -> i64
+! CHECK-NEXT: %[[waitPresent:.*]] = arith.cmpi ne, %[[waitCast]], %c0_i64 : i64
+! CHECK-NEXT: %[[wait:.*]] = fir.if %[[waitPresent]] -> (i1) {
+! CHECK-NEXT: %[[waitLoaded:.*]] = fir.load %[[waitDeclare]] : !fir.ref<!fir.logical<4>>
+! CHECK-NEXT: %[[waitTrueVal:.*]] = fir.convert %[[waitLoaded]] : (!fir.logical<4>) -> i1
+! CHECK-NEXT: fir.result %[[waitTrueVal]] : i1
+! CHECK-NEXT: } else {
+! CHECK-NEXT: fir.result %true : i1
+! CHECK-NEXT: }
! CHECK: %[[command:.*]] = fir.convert %[[commandBox]] : (!fir.box<!fir.char<1,?>>) -> !fir.box<none>
-! CHECK-NEXT: %[[wait:.*]] = fir.convert %[[waitLoaded]] : (!fir.logical<4>) -> i1
! CHECK-NEXT: %[[exitstat:.*]] = fir.convert %[[exitstatBox]] : (!fir.box<i32>) -> !fir.box<none>
! CHECK-NEXT: %[[cmdstat:.*]] = fir.convert %[[cmdstatBox]] : (!fir.box<i32>) -> !fir.box<none>
! CHECK-NEXT: %[[cmdmsg:.*]] = fir.convert %[[cmdmsgBox]] : (!fir.box<!fir.char<1,?>>) -> !fir.box<none>
diff --git a/flang/test/Lower/Intrinsics/execute_command_line.f90 b/flang/test/Lower/Intrinsics/execute_command_line.f90
index 1b65bbd5e1550ce..92234c39110ca42 100644
--- a/flang/test/Lower/Intrinsics/execute_command_line.f90
+++ b/flang/test/Lower/Intrinsics/execute_command_line.f90
@@ -11,26 +11,33 @@ subroutine all_args(command, isWait, exitVal, cmdVal, msg)
INTEGER :: exitVal, cmdVal
LOGICAL :: isWait
call execute_command_line(command, isWait, exitVal, cmdVal, msg)
-! CHECK: %[[cmdstatsDeclear:.*]] = fir.declare %[[cmdstatArg]] {uniq_name = "_QFall_argsEcmdval"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK: %[[cmdstatsDeclare:.*]] = fir.declare %[[cmdstatArg]] {uniq_name = "_QFall_argsEcmdval"} : (!fir.ref<i32>) -> !fir.ref<i32>
! CHECK-NEXT: %[[commandUnbox:.*]]:2 = fir.unboxchar %[[commandArg]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK-NEXT: %[[commandCast:.*]] = fir.convert %[[commandUnbox]]#0 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.char<1,30>>
-! CHECK-NEXT: %[[commandDeclear:.*]] = fir.declare %[[commandCast]] typeparams %c30 {uniq_name = "_QFall_argsEcommand"} : (!fir.ref<!fir.char<1,30>>, index) -> !fir.ref<!fir.char<1,30>>
-! CHECK-NEXT: %[[exitstatDeclear:.*]] = fir.declare %[[exitstatArg]] {uniq_name = "_QFall_argsEexitval"} : (!fir.ref<i32>) -> !fir.ref<i32>
-! CHECK-NEXT: %[[waitDeclear:.*]] = fir.declare %[[waitArg]] {uniq_name = "_QFall_argsEiswait"} : (!fir.ref<!fir.logical<4>>) -> !fir.ref<!fir.logical<4>>
+! CHECK-NEXT: %[[commandDeclare:.*]] = fir.declare %[[commandCast]] typeparams %c30 {uniq_name = "_QFall_argsEcommand"} : (!fir.ref<!fir.char<1,30>>, index) -> !fir.ref<!fir.char<1,30>>
+! CHECK-NEXT: %[[exitstatDeclare:.*]] = fir.declare %[[exitstatArg]] {uniq_name = "_QFall_argsEexitval"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK-NEXT: %[[waitDeclare:.*]] = fir.declare %[[waitArg]] {uniq_name = "_QFall_argsEiswait"} : (!fir.ref<!fir.logical<4>>) -> !fir.ref<!fir.logical<4>>
! CHECK-NEXT: %[[cmdmsgUnbox:.*]]:2 = fir.unboxchar %[[cmdmsgArg]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK-NEXT: %[[cmdmsgCast:.*]] = fir.convert %[[cmdmsgUnbox]]#0 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.char<1,30>>
-! CHECK-NEXT: %[[cmdmsgDeclear:.*]] = fir.declare %[[cmdmsgCast]] typeparams %c30 {uniq_name = "_QFall_argsEmsg"} : (!fir.ref<!fir.char<1,30>>, index) -> !fir.ref<!fir.char<1,30>>
-! CHECK-NEXT: %[[commandBox:.*]] = fir.embox %[[commandDeclear]] : (!fir.ref<!fir.char<1,30>>) -> !fir.box<!fir.char<1,30>>
-! CHECK-NEXT: %[[waitLoaded:.*]] = fir.load %[[waitDeclear]] : !fir.ref<!fir.logical<4>>
-! CHECK-NEXT: %[[exitstatBox:.*]] = fir.embox %[[exitstatDeclear]] : (!fir.ref<i32>) -> !fir.box<i32>
-! CHECK-NEXT: %[[cmdstatBox:.*]] = fir.embox %[[cmdstatsDeclear]] : (!fir.ref<i32>) -> !fir.box<i32>
-! CHECK-NEXT: %[[cmdmsgBox:.*]] = fir.embox %[[cmdmsgDeclear]] : (!fir.ref<!fir.char<1,30>>) -> !fir.box<!fir.char<1,30>>
+! CHECK-NEXT: %[[cmdmsgDeclare:.*]] = fir.declare %[[cmdmsgCast]] typeparams %c30 {uniq_name = "_QFall_argsEmsg"} : (!fir.ref<!fir.char<1,30>>, index) -> !fir.ref<!fir.char<1,30>>
+! CHECK-NEXT: %[[commandBox:.*]] = fir.embox %[[commandDeclare]] : (!fir.ref<!fir.char<1,30>>) -> !fir.box<!fir.char<1,30>>
+! CHECK-NEXT: %[[exitstatBox:.*]] = fir.embox %[[exitstatDeclare]] : (!fir.ref<i32>) -> !fir.box<i32>
+! CHECK-NEXT: %[[cmdstatBox:.*]] = fir.embox %[[cmdstatsDeclare]] : (!fir.ref<i32>) -> !fir.box<i32>
+! CHECK-NEXT: %[[cmdmsgBox:.*]] = fir.embox %[[cmdmsgDeclare]] : (!fir.ref<!fir.char<1,30>>) -> !fir.box<!fir.char<1,30>>
+! CHECK-NEXT: %[[waitCast:.*]] = fir.convert %[[waitDeclare]] : (!fir.ref<!fir.logical<4>>) -> i64
+! CHECK-NEXT: %[[waitPresent:.*]] = arith.cmpi ne, %[[waitCast]], %c0_i64 : i64
+! CHECK-NEXT: %[[wait:.*]] = fir.if %[[waitPresent]] -> (i1) {
+! CHECK-NEXT: %[[waitLoaded:.*]] = fir.load %[[waitDeclare]] : !fir.ref<!fir.logical<4>>
+! CHECK-NEXT: %[[waitTrueVal:.*]] = fir.convert %[[waitLoaded]] : (!fir.logical<4>) -> i1
+! CHECK-NEXT: fir.result %[[waitTrueVal]] : i1
+! CHECK-NEXT: } else {
+! CHECK-NEXT: fir.result %true : i1
+! CHECK-NEXT: }
! CHECK: %[[command:.*]] = fir.convert %[[commandBox]] : (!fir.box<!fir.char<1,30>>) -> !fir.box<none>
-! CHECK-NEXT: %[[wait:.*]] = fir.convert %[[waitLoaded]] : (!fir.logical<4>) -> i1
! CHECK-NEXT: %[[exitstat:.*]] = fir.convert %[[exitstatBox]] : (!fir.box<i32>) -> !fir.box<none>
! CHECK-NEXT: %[[cmdstat:.*]] = fir.convert %[[cmdstatBox]] : (!fir.box<i32>) -> !fir.box<none>
! CHECK-NEXT: %[[cmdmsg:.*]] = fir.convert %[[cmdmsgBox]] : (!fir.box<!fir.char<1,30>>) -> !fir.box<none>
-! CHECK: %[[VAL_21:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %[[wait]], %[[exitstat]], %[[cmdstat]], %[[cmdmsg]], %[[VAL_20:.*]], %c13_i32) fastmath<contract> : (!fir.box<none>, i1, !fir.box<none>, !fir.box<none>, !fir.box<none>, !fir.ref<i8>, i32) -> none
+! CHECK: %[[VAL_22:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %[[wait]], %[[exitstat]], %[[cmdstat]], %[[cmdmsg]], %[[VAL_20:.*]], %c13_i32) fastmath<contract> : (!fir.box<none>, i1, !fir.box<none>, !fir.box<none>, !fir.box<none>, !fir.ref<i8>, i32) -> none
! CHECK-NEXT: return
end subroutine all_args
@@ -39,7 +46,7 @@ end subroutine all_args
subroutine only_command_default_wait_true(command)
CHARACTER(30) :: command
call execute_command_line(command)
-! CHECK-NEXT: %c41_i32 = arith.constant 41 : i32
+! CHECK-NEXT: %c48_i32 = arith.constant 48 : i32
! CHECK-NEXT: %true = arith.constant true
! CHECK-NEXT: %c30 = arith.constant 30 : index
! CHECK-NEXT: %[[commandUnbox:.*]]:2 = fir.unboxchar %[[cmdArg]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
@@ -48,6 +55,6 @@ subroutine only_command_default_wait_true(command)
! CHECK-NEXT: %[[commandBox:.*]] = fir.embox %[[commandDeclare]] : (!fir.ref<!fir.char<1,30>>) -> !fir.box<!fir.char<1,30>>
! CHECK-NEXT: %[[absent:.*]] = fir.absent !fir.box<none>
! CHECK: %[[command:.*]] = fir.convert %[[commandBox]] : (!fir.box<!fir.char<1,30>>) -> !fir.box<none>
-! CHECK: %[[VAL_21:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %true, %[[absent]], %[[absent]], %[[absent]], %[[VAL_7:.*]], %c41_i32) fastmath<contract> : (!fir.box<none>, i1, !fir.box<none>, !fir.box<none>, !fir.box<none>, !fir.ref<i8>, i32) -> none
+! CHECK: %[[VAL_8:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %true, %[[absent]], %[[absent]], %[[absent]], %[[VAL_7:.*]], %c48_i32) fastmath<contract> : (!fir.box<none>, i1, !fir.box<none>, !fir.box<none>, !fir.box<none>, !fir.ref<i8>, i32) -> none
! CHECK-NEXT: return
end subroutine only_command_default_wait_true
|
Comment on lines
+2933
to
+2951
| mlir::Type i1Ty = builder.getI1Type(); | ||
| mlir::Value waitAddr = fir::getBase(wait); | ||
| mlir::Value waitIsPresentAtRuntime = | ||
| builder.genIsNotNullAddr(loc, waitAddr); | ||
| waitBool = builder | ||
| .genIfOp(loc, {i1Ty}, waitIsPresentAtRuntime, | ||
| /*withElseRegion=*/true) | ||
| .genThen([&]() { | ||
| auto waitLoad = builder.create<fir::LoadOp>(loc, waitAddr); | ||
| mlir::Value cast = | ||
| builder.createConvert(loc, i1Ty, waitLoad); | ||
| builder.create<fir::ResultOp>(loc, cast); | ||
| }) | ||
| .genElse([&]() { | ||
| mlir::Value trueVal = builder.createBool(loc, true); | ||
| builder.create<fir::ResultOp>(loc, trueVal); | ||
| }) | ||
| .getResults()[0]; | ||
| } |
There was a problem hiding this comment.
Is there similar handling for any other intrinsic?
There was a problem hiding this comment.
There is one for GetEnvironmentVariable for trim bool variable,
There was a problem hiding this comment.
https://reviews.llvm.org/D123388 explains the logic behind this code.
There was a problem hiding this comment.
Thanks for the pointers @yi-wu-arm @luporl
psteinfeld
reviewed
Jan 17, 2024
it turns out that it doesn't matter if the optional input are pass in or not, when compiling it is 5
kiranchandramohan
approved these changes
Jan 23, 2024
Comment on lines
+2933
to
+2951
| mlir::Type i1Ty = builder.getI1Type(); | ||
| mlir::Value waitAddr = fir::getBase(wait); | ||
| mlir::Value waitIsPresentAtRuntime = | ||
| builder.genIsNotNullAddr(loc, waitAddr); | ||
| waitBool = builder | ||
| .genIfOp(loc, {i1Ty}, waitIsPresentAtRuntime, | ||
| /*withElseRegion=*/true) | ||
| .genThen([&]() { | ||
| auto waitLoad = builder.create<fir::LoadOp>(loc, waitAddr); | ||
| mlir::Value cast = | ||
| builder.createConvert(loc, i1Ty, waitLoad); | ||
| builder.create<fir::ResultOp>(loc, cast); | ||
| }) | ||
| .genElse([&]() { | ||
| mlir::Value trueVal = builder.createBool(loc, true); | ||
| builder.create<fir::ResultOp>(loc, trueVal); | ||
| }) | ||
| .getResults()[0]; | ||
| } |
There was a problem hiding this comment.
Thanks for the pointers @yi-wu-arm @luporl
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an optional bool in passed as optional,
isStaticallyPresent()will actually return true.By changing wait from
asValuetoasAddrand add some check can solve this.Fixes: #77984