Skip to content

fix optional wait wrongly treated as false #78149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 23, 2024
Merged

fix optional wait wrongly treated as false #78149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
485255c
fix optional wait wrongly treated as false
yiwu0b11 Jan 15, 2024
04aeae0
clang-format
yiwu0b11 Jan 15, 2024
ba402a9
remove debugging line
yiwu0b11 Jan 15, 2024
1461547
assertion and test fixes
yiwu0b11 Jan 15, 2024
b664056
test fixes and use variable to capture constant
yiwu0b11 Jan 15, 2024
6e731b7
change assert back to args.size()==5
yiwu0b11 Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions flang/lib/Optimizer/Builder/IntrinsicCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}},
Expand Down Expand Up @@ -2914,7 +2914,9 @@ IntrinsicLibrary::genEoshift(mlir::Type resultType,
void IntrinsicLibrary::genExecuteCommandLine(
llvm::ArrayRef<fir::ExtendedValue> args) {
assert(args.size() == 5);

mlir::Value command = fir::getBase(args[0]);
// Optional arguments: wait, exitstat, cmdstat, cmdmsg.
const fir::ExtendedValue &wait = args[1];
const fir::ExtendedValue &exitstat = args[2];
const fir::ExtendedValue &cmdstat = args[3];
Expand All @@ -2925,9 +2927,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];
}
Comment on lines +2934 to +2952
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there similar handling for any other intrinsic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one for GetEnvironmentVariable for trim bool variable,

llvm-project/flang/lib/Optimizer/Builder/IntrinsicCall.cpp

Line 3187 in cfa30fa

mlir::Value trim;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://reviews.llvm.org/D123388 explains the logic behind this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers @yi-wu-arm @luporl


mlir::Value exitstatBox =
isStaticallyPresent(exitstat)
? fir::getBase(exitstat)
Expand Down
25 changes: 14 additions & 11 deletions flang/test/Lower/Intrinsics/execute_command_line-optional.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ subroutine all_args_optional(command, isWait, exitVal, cmdVal, msg)
LOGICAL, OPTIONAL :: isWait
! Note: command is not optional in execute_command_line and must be present
call execute_command_line(command, isWait, exitVal, cmdVal, msg)
! CHECK: %[[cmdstatDeclare:.*]] = fir.declare %[[cmdstatArg]] {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "_QFall_args_optionalEcmdval"} : (!fir.ref<i32>) -> !fir.ref<i32>
! CHECK-NEXT: %[[c14:.*]] = arith.constant 14 : i32
! CHECK-NEXT: %true = arith.constant true
! CHECK-NEXT: %[[c0:.*]] = arith.constant 0 : i64
! CHECK-NEXT: %[[cmdstatDeclare:.*]] = fir.declare %[[cmdstatArg]] {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "_QFall_args_optionalEcmdval"} : (!fir.ref<i32>) -> !fir.ref<i32>
! CHECK-NEXT: %[[commandUnbox:.*]]:2 = fir.unboxchar %[[commandArg]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK-NEXT: %[[commandDeclare:.*]] = fir.declare %[[commandUnbox]]#0 typeparams %[[commandUnbox]]#1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "_QFall_args_optionalEcommand"} : (!fir.ref<!fir.char<1,?>>, index) -> !fir.ref<!fir.char<1,?>>
! CHECK-NEXT: %[[commandBoxTemp:.*]] = fir.emboxchar %[[commandDeclare]], %[[commandUnbox]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.boxchar<1>
Expand All @@ -21,18 +24,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>
Expand All @@ -41,11 +36,19 @@ 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
! 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>
! CHECK: %[[VAL_30:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %[[wait]], %[[exitstat]], %[[cmdstat]], %[[cmdmsg]], %[[VAL_29:.*]], %c14_i32) fastmath<contract> : (!fir.box<none>, i1, !fir.box<none>, !fir.box<none>, !fir.box<none>, !fir.ref<i8>, i32) -> none
! CHECK: %[[VAL_30:.*]] = fir.call @_FortranAExecuteCommandLine(%[[command]], %[[wait]], %[[exitstat]], %[[cmdstat]], %[[cmdmsg]], %[[VAL_29:.*]], %[[c14]]) 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_optional
43 changes: 27 additions & 16 deletions flang/test/Lower/Intrinsics/execute_command_line.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,37 @@ 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-NEXT: %[[c13:.*]] = arith.constant 13 : i32
! CHECK-NEXT: %true = arith.constant true
! CHECK-NEXT: %[[c0:.*]] = arith.constant 0 : i64
! CHECK-NEXT: %[[c30:.*]] = arith.constant 30 : index
! CHECK-NEXT: %[[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
! 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]]) 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

Expand All @@ -39,15 +50,15 @@ 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: %[[c52:.*]] = arith.constant 52 : i32
! CHECK-NEXT: %true = arith.constant true
! CHECK-NEXT: %c30 = arith.constant 30 : index
! CHECK-NEXT: %[[c30:.*]] = arith.constant 30 : index
! CHECK-NEXT: %[[commandUnbox:.*]]:2 = fir.unboxchar %[[cmdArg]] : (!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: %[[commandDeclare:.*]] = fir.declare %[[commandCast]] typeparams %c30 {uniq_name = "_QFonly_command_default_wait_trueEcommand"} : (!fir.ref<!fir.char<1,30>>, index) -> !fir.ref<!fir.char<1,30>>
! CHECK-NEXT: %[[commandDeclare:.*]] = fir.declare %[[commandCast]] typeparams %[[c30]] {uniq_name = "_QFonly_command_default_wait_trueEcommand"} : (!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: %[[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:.*]], %[[c52]]) 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