This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Give fir.if RegionBranchOpInterface
ClosedPublic

Authored by SBallantyne on Mar 2 2023, 7:35 AM.

Details

Summary

fir.if currently isn't treated as a 'proper' conditional, so passes are unable to determine which regions are executed at times.

This patch gives fir.if this interface, which shouldn't do too much on its own but should allow future changes to take advantage
for various purposes

Diff Detail

Event Timeline

SBallantyne created this revision.Mar 2 2023, 7:35 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 2 2023, 7:35 AM
SBallantyne requested review of this revision.Mar 2 2023, 7:35 AM

Cleanup changes

tblah added inline comments.Mar 2 2023, 7:44 AM
flang/lib/Optimizer/Dialect/FIROps.cpp
22

nit: this can go

3431

In Fortran/Flang are if conditions strictly true == 1 or is true any non-zero value?

3448

I know you just copied this code, but it strange that this casts to a BoolAttr whereas the other to an IntegerAttr. If flang allows true as any non-zero value, you will need to make sure that is respected in any cast to BoolAttr.

flang/lib/Optimizer/Transforms/StackArrays.cpp
402

nit: remove this

flang/test/Transforms/stack-arrays.fir
51

Please could you make this a new test instead of changing the existing one. Reviewers on the stack arrays pass wanted to make sure it worked on scf.if so we should keep the test for that. Furthermore, this is testing different behavior than the old test (a valid conversion vs ensuring we don't modify an invalid conversion).

tblah added inline comments.Mar 2 2023, 7:45 AM
flang/lib/Optimizer/Dialect/FIROps.cpp
22

I was commenting on the old version of the patch where this was commented out. As you have uncommitted it, I presume it is actually a required include.

SBallantyne marked an inline comment as done.

Add scf.if test back

SBallantyne marked an inline comment as done.Mar 2 2023, 8:30 AM
SBallantyne added inline comments.
flang/lib/Optimizer/Dialect/FIROps.cpp
3431

As fir.if takes an I1, it should be fine to check for 1 instead of 0 at this point, any integer != 0 should have been converted to 1 by now.

3448

Similar to above, while it is inconsistent that we use cast to bool vs casting to int it seems like BoolAttr follows the same rules as flang that any non-zero value will be true.

tblah added a comment.Mar 2 2023, 8:33 AM

Thanks for your changes. LGTM but wait for somebody else to look over it

flang/test/Transforms/stack-arrays.fir
71

nit: I suspect you won't be allowed two functions named dfa2

It makes sense to me to add this interface to fir.if and LGTM. Just adding @vzakhari here to ensure I am not missing something.

flang/include/flang/Optimizer/Dialect/FIROps.td
2150
2150

We probably have to add RecursiveMemoryEffects as well.

2172

Nit: In a subsequent patch you can change this to MaxSizedRegion<1>.

SBallantyne marked 4 inline comments as done.

Amend test name, add RecursiveMemoryEffects trait and limit fir.if to 1 else region

Clang format has changed in between patches, appease it

One minor thing, otherwise, looks good.

flang/include/flang/Optimizer/Dialect/FIROps.td
2150

nit: getNumRegionInvocations does not seem to be an interface method of RegionBranchOpInterface. Moreover, you do not seem to override it, so this mention is probably redundant (here and in scf.if).

vzakhari accepted this revision.Mar 7 2023, 2:15 PM
This revision is now accepted and ready to land.Mar 7 2023, 2:15 PM

Remove redundant getNumRegionInvocations from scf.if and fir.if

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 9:07 AM

Remove change to scf

vzakhari accepted this revision.Mar 8 2023, 9:41 AM

Thanks!

This revision was automatically updated to reflect the committed changes.