Page MenuHomePhabricator

[FLANG][NFC]Use RTNAME instead of hard-coding for simplify intrinsics
ClosedPublic

Authored by Leporacanthicus on Aug 25 2022, 6:21 AM.

Details

Summary

Use the RTNMAE macro (via stringify macros) to generate the name
strings for runtime functions, instead of using strings.
The sequence of macros generate exactly the same string as the
ones used previously, but this will support future changes in
runtime function names.

No functional change.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: mehdi_amini. · View Herald Transcript
Leporacanthicus requested review of this revision.Aug 25 2022, 6:21 AM
vzakhari added inline comments.Aug 26 2022, 3:02 PM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
32

Is this files really needed?

170–172

I am not sure RTName prefix really useful here. Maybe use something like FRT (standing for Fortran runtime) or more verbose FortranRuntime.

Revew comment fixes:

  • Removed unused header file
  • Changed function names from RTName to Runtime to better reflect what they do.
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
32

Yes, we need to include entry-names.h , as that is where RTNAME is defined. Including the RTBuilder.h is not needed, and I've removed that. I originally intended to use a macro from RTBuilder.h, that did the stringify, but I decided that

Interestingly, RTBuilder.h uses RTNAME without including the header it relies on - but all the relevant flang/Runtime/*.h do include entry-names.h, so it works.

Maybe entry-names.h should be moved (or move the RTNAME macro - it's the ONLY thing in entry-names.h)?

Suggestions welcome if this should be changed.

170–172

I wasn't exactly happy with that name either, but couldn't think of something better at the time. I've gone for genRumtimeSomething as a compromise between long and cryptic variants. I don't think it's that far-fetched to understand that Runtime here means Fortran Runtime.

LGTM, except for the buildbot failures.

flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
32

I guess entry-names.h was created exactly to expose RTNAME for different components in flang. Not all of them require RTBuilder functionality, so I guess it is okay as-is.

170–172

Looks good now. Thanks.

Rebase to latest llvm/main

LGTM, except for the buildbot failures.

Yeah, I didn't link it to the previous commit that it depends on. As that as gone in and this is now rebased, should be good to go on Monday! :)

vzakhari accepted this revision.Sep 2 2022, 8:01 PM
This revision is now accepted and ready to land.Sep 2 2022, 8:01 PM