This is an archive of the discontinued LLVM Phabricator instance.

[flang] add memory effects interface to (hl)fir.declare
ClosedPublic

Authored by tblah on Aug 10 2023, 8:16 AM.

Details

Summary

These operations should have no memory effect, but doing so causes the
declare to be removed by dead code elimination if the result value is
unused. fir.declare is intended to be used to generate debug information
about variables. Debug information may still be desirable even about
unused variables, so we don't want to remove the declare operations when
performing dead code elimination.

Diff Detail

Event Timeline

tblah created this revision.Aug 10 2023, 8:16 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 10 2023, 8:16 AM
tblah requested review of this revision.Aug 10 2023, 8:16 AM

I would prefer it to be modeled as a write effect of a new "debugger" resource. And the write effect should be only reported under -g.

tblah updated this revision to Diff 549325.Aug 11 2023, 3:48 AM

Thanks for review.

I've updated to use MemWrite to a debugging resource. It isn't easy to make
memory effects runtime conditional so I haven't made it conditional on -g,
which I think is good because otherwise people might not test and optimizations
won't run with -g.

I agree that writing to a debugging resource sounds more right linguisticly. But
I think it is unfortunate that now we have to remember to code in checks for
this debugging resource into optimization passes (if a pass looks for writes
which might alias with some value without looking at the resource, it will have
to assume this operation does because it is a write on an unspecified value).
Fortunately, we don't need upstream mlir passes (like CSE) to work on DeclareOp.

Semantically, an allocation is a better fit because you can't remove it but it
is known not to effect anything else.

tblah edited the summary of this revision. (Show Details)Aug 11 2023, 3:49 AM
vzakhari accepted this revision.Aug 11 2023, 9:59 AM

Thank you, Tom! Let's try keeping it this way. If this causes performance issues, we will revisit it.

It would be great to have some LIT test for this, e.g. like mlir/test/IR/test-side-effects.mlir.

This revision is now accepted and ready to land.Aug 11 2023, 9:59 AM

Thank you, Tom! Let's try keeping it this way. If this causes performance issues, we will revisit it.

It would be great to have some LIT test for this, e.g. like mlir/test/IR/test-side-effects.mlir.

It'll be covered in tests for https://reviews.llvm.org/D157107 when I update it.

But yes, ideally all FIR and HLFIR ops would have their side effects tested like that

It'll be covered in tests for https://reviews.llvm.org/D157107 when I update it.

Sounds good. Thank you!

This revision was automatically updated to reflect the committed changes.