This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Add Minloc to simplify intrinsics pass
ClosedPublic

Authored by SBallantyne on Feb 15 2023, 6:58 AM.

Details

Summary

This patch adds minloc to the simplify intrinsics pass, supporting calls with KIND or MASK arguments while calls which have BACK, DIM or have a CHARACTER input array are rejected. This patch is targeting exchange2, and in benchmarks provides a ~11% improvement in performance.

Also included are some minor style changes / cleanup in simplifyIntrinsics.cpp.

Diff Detail

Event Timeline

SBallantyne created this revision.Feb 15 2023, 6:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
SBallantyne requested review of this revision.Feb 15 2023, 6:58 AM
SBallantyne edited the summary of this revision. (Show Details)Feb 15 2023, 7:02 AM

Minor bug fix to avoid using uninitialised variables in for loops

vzakhari added inline comments.Feb 15 2023, 5:55 PM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
156

This will be 'declared but not used' issue in release builds. I think you can just use Op::getOperationName() inside LLVM_DEBUG.

566

Is it really necessary to pass the indices array by value?

Please use Fortran::common::maxRank instead of 15.

644

typo: to to

1193
SBallantyne marked 4 inline comments as done.

Style changes and some small improvements

tblah added inline comments.Feb 16 2023, 6:22 AM
flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp
38

Nit: what is this used for?

39

Nit: what is this used for?

Fix an issue where input arrays containing only huge(i) would return 0 instead of 1.

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

I believe this was used to import mlir::Operation so I've changed it to just import that file rather than getting it through TypeUtilites.

39

This is needed for the reference to mlir::Pass in the function at the bottom of the file:

std::unique_ptr<mlir::Pass> fir::createSimplifyIntrinsicsPass() {
  return std::make_unique<SimplifyIntrinsicsPass>();
}

Update fir tests to match new generated code and change naming scheme to avoid confusing logical ranks

Generally looks OK, just some minor nitpicks.

Feel free to ignore my nit-picking, and I'm definitely not expecting this to depend on me approving it next week when I'm back in the office.

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

Maybe auto defOp = op->getOperand(0).getDefiningOp(); and then use defOp here and two lines below? [If my comment below about is-one-of type thing can't be implemented].

217

Here, and in findBoxDef it feels like what we would like is some sort of if op is one of <list of types> then give me the result - I don't know exactly what you'd need to do for this to work, and it may not be at all worth it - it's just my sort of feeling that "this seems to repeat the same thing over and over".

504–505

Instead of doing if(isa<fir::IfOp>(...)) and then dyn_cast<fir::IfOp>(...), you can just do if(fir::IfOp = dyn_cast<fir::IfOp>(...)).

815–816

This is the same for both if and else, right? So you could move the fir::IfOp ifOp down, and initalize it in one place, rather than duplicate it?

885–889

This should go with the lines above, we can probably make it bool hasMask = maskRank > 0; [Well, or move all of it down here, since I don't think it's used above]

1224

Extreme nitpick: here and above, the reference oprerator isn't really needed to pass the vlaue to the function, is it? We'er not changing the vlaues of the local variables in the lambda function?

SBallantyne marked 4 inline comments as done.

Change findBoxDef and findMaskDef to use a common method findDef

This revision is now accepted and ready to land.Feb 23 2023, 8:48 PM
This revision was automatically updated to reflect the committed changes.
SBallantyne marked an inline comment as done.