This is an archive of the discontinued LLVM Phabricator instance.

Bitwise comparison intrinsics
ClosedPublic

Authored by tarunprabhu on Jun 14 2022, 3:44 PM.

Details

Summary

This patch implements lowering for the F08 bitwise comparison intrinsics (BGE, BGT, BLE and BLT).

This does not create any runtime functions since the functionality is simple enough to carry out in IR.

The existing semantic check has been changed because it unconditionally converted the arguments to the largest possible integer type. This resulted in the argument with the smaller bit-size being sign-extended. However, the standard requires the argument with the smaller bit-size to be zero-extended.

Diff Detail

Event Timeline

tarunprabhu created this revision.Jun 14 2022, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 3:44 PM
tarunprabhu requested review of this revision.Jun 14 2022, 3:44 PM
klausler requested changes to this revision.Jun 27 2022, 12:21 PM
klausler added inline comments.
flang/lib/Evaluate/fold-logical.cpp
120–121

What about calls to BGE & al. that must be folded at compilation time in order to process a constant expression in a declaration?

This revision now requires changes to proceed.Jun 27 2022, 12:21 PM
vdonaldson added inline comments.Jun 27 2022, 12:42 PM
flang/lib/Lower/IntrinsicCall.cpp
464–467

These intermediate declarations can be skipped in favor of direct calls to genBitwiseCompare, the definition of which is then is no longer a helper function and can be moved down in the code. Compare the code for lge, lgt, lle, llt.

The intermediate declarations have been skipped in favor of direct calls to genBitwiseCompare as suggested by @vdonaldson

If both the arguments to the intrinsics are constant, fold the constant. The constants are zero-extended to the largest possible integer.

The tests have been updated to reflect these changes.

Thanks for the update ! A few more nits in the constant expression folding code.

flang/lib/Evaluate/fold-logical.cpp
23

Scalar<LargestInt>::ConvertUnsigned should also work here and would be more generic.

24

The bitwise intrinsic operands may be array constant, and this case must also be foldable in constant expression.
In that case, I think you could use c->values() to go through all the element values and convert them, and to create a new Constant<LargestInt>{std::move(zextValues), std::move(c->shape())} passing the previous shape.

84

The flang code tends to prefer usage of std::visit when possible. The semantics code also tends to favor const& over passing via raw pointers. Here, I think you could do:

if (BOZLiteralConstant *...) {
 ...
} else if (auto *expr{UnwrapExpr<Expr<SomeInteger>>(args[i])}) {
  std::visit([&](const auto& typedIntExpr){
     using IntT = typename std::decay_t<decltype(x)>::Result;
     if (auto *c{UnwrapConstantValue<IntT >(typedIntExpr)}) {
        constArgs[i] = zeroExtend(*c);
     }
   }, expr->u) {
  }
}

The big advantage is that you do not have to assume too much about the list of possible integer types here.

vdonaldson added inline comments.Jul 5 2022, 1:33 PM
flang/lib/Lower/IntrinsicCall.cpp
2121

Thanks for simplifying these definitions. Most of these intrinsics are placed alphabetically. It would be nice to follow that here, and move these down between AINT and BTEST

tarunprabhu marked an inline comment as done.

Updated patch to incorporate the comments of @jeanPerier and @vdonaldson .

The Intrinsic.cpp changes look ok to me. Thanks for the updates!

jeanPerier accepted this revision.Jul 7 2022, 12:09 AM

Be sure to run clang-format on your change in lib/Evaluate/fold-logical.cpp (bots seems to complain about formatting there) before merging. Otherwise looks good to me.
Thanks a lot for implementing the bitwise intrinsic lowering, solving the folding issue, and dealing with all the comments !

tarunprabhu marked 2 inline comments as done.

Updated diff after running clang-format.

@klausler, could you take a look and let me know if this is ok.

tarunprabhu marked 2 inline comments as done.Jul 7 2022, 12:57 PM
tarunprabhu added inline comments.
flang/lib/Evaluate/fold-logical.cpp
120–121

Do they need to be folded before lowering to FIR? The way this is implemented, the constants get folded during lowering from FIR to LLVM-IR.

120–121

Oh, sorry, I misunderstood your comment. I only tested against constant expressions, but not in declarations. The latter, indeed, doesn't work.

tarunprabhu marked 2 inline comments as done.Jul 7 2022, 12:58 PM
klausler added inline comments.Jul 7 2022, 1:20 PM
flang/lib/Evaluate/fold-logical.cpp
16

In semantics, function names are capitalized.

18

can be "const auto &v : ..."

80

The initializer is not necessary -- it matches the default behavior.

85

common::visit has replaced std::visit

96

The ".has_value()" is not needed, and we use "&&" rather than "and".

109

We always use braces in semantics around if/for/while bodies, even when they are single statements.

Updated patch to address comments by @klausler

tarunprabhu marked 6 inline comments as done.Jul 7 2022, 2:16 PM

Please add tests for folding the bit comparison intrinsic functions.

flang/lib/Evaluate/fold-logical.cpp
88

"const auto *"

Please add tests for folding the bit comparison intrinsic functions.

I'm not sure what you mean. Do you want me to add tests where all arguments to the intrinsic functions are known constants?

Please add tests for folding the bit comparison intrinsic functions.

I'm not sure what you mean. Do you want me to add tests where all arguments to the intrinsic functions are known constants?

Yes, that's right. See flang/test/Evaluate/*.f90 to see other constant folding tests. The general technique is to put foldable expressions as initializers of named constants in modules, with LOGICAL named constants used to verify their results. If an expression doesn't get folded, the compilation of the module will fail, and if the folded value is incorrect, the module file won't contain a definition of the LOGICAL variable as .true.

Added folding tests.

I have covered some of the obviously "interesting" cases, but there may be some that I have missed.

klausler accepted this revision.Jul 8 2022, 12:13 PM

Well done; thank you!

This revision is now accepted and ready to land.Jul 8 2022, 12:13 PM

I don't have commit access. Can someone merge this?

Happy to merge this for you, but you could you add RUN lines with flang-new first?

I don't have commit access. Can someone merge this?

You should request access.

Add flang driver to tests.

Add flang driver to tests.

Thanks! What e-mail address should I use when committing this change?

Add flang driver to tests.

Thanks! What e-mail address should I use when committing this change?

Thanks! You can use tarunprabhu@gmail.com

This revision was landed with ongoing or failed builds.Jul 19 2022, 9:42 AM
Closed by commit rG7709f12ed08d: Bitwise comparison intrinsics (authored by tarunprabhu, committed by awarzynski). · Explain Why
This revision was automatically updated to reflect the committed changes.