This is an archive of the discontinued LLVM Phabricator instance.

[flang] Implement the runtime portion of the CSHIFT intrinsic
ClosedPublic

Authored by PeteSteinfeld on Jul 19 2021, 11:23 AM.

Details

Summary

This change fixes a bug in the runtime portion of the CSHIFT intrinsic that happens when the value of the SHIFT argument is negative.

Diff Detail

Event Timeline

PeteSteinfeld requested review of this revision.Jul 19 2021, 11:23 AM
PeteSteinfeld created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 11:23 AM
PeteSteinfeld added a project: Restricted Project.
mleair added inline comments.Jul 19 2021, 11:40 AM
flang/runtime/transformational.cpp
190

llvm style recommends no braces for single statement blocks.

PeteSteinfeld added inline comments.Jul 19 2021, 12:12 PM
flang/runtime/transformational.cpp
190

I didn't think that we were following this recommendation for flang code. This file, in particular is full of single statement blocks that are enclosed by braces. There's one at line 162, another at line 167, line 232, ...

jeanPerier accepted this revision.Jul 20 2021, 1:00 AM
jeanPerier added inline comments.
flang/runtime/transformational.cpp
177

I am shared. I understand why you are doing this interface change to have simpler lowering code, but using a descriptor instead of a simple integer has user runtime cost (creating the descriptor and reading it).
In this specific case, the extra runtime cost coming from the descriptor might be negligible compared to what CSHIFT is doing, and it's hard for me to complain about performance without any numbers/benchmark. So this change is probably OK.
In general, think we should find solution in lowering to keep lowering code as simple as possible rather than moving the cost to the user.

This revision is now accepted and ready to land.Jul 20 2021, 1:00 AM
PeteSteinfeld added inline comments.Jul 21 2021, 12:02 PM
flang/runtime/transformational.cpp
177

I'm not sure what previous problem I was running into, but my latest implementation doesn't require a change to the interface.

Responding to Jean's comments by reverting my change to the vector version of
CSHIFT.

PeteSteinfeld edited the summary of this revision. (Show Details)Jul 21 2021, 12:31 PM

Trying to ensure that things are in sync.

Adding needed changes in .../flang/unittests/RuntimeGtest/Transformational.cpp.

mleair accepted this revision.Jul 21 2021, 1:10 PM

LGTM

flang/runtime/transformational.cpp
190

OK

This revision was landed with ongoing or failed builds.Jul 21 2021, 1:39 PM
This revision was automatically updated to reflect the committed changes.