[Open64-devel] cleanup patch to remove PROMPF code [LNO+]

Discussion:

David Coakley

2012-02-20 04:07:12 UTC

Hi all,

Here is a patch that removes dead code related to the obsolete PROMPF
feature. The patch is farily large but the changes are
straightforward. Removing this code makes the remaining code easier
to understand, especially for those new to Open64.

Could one of the gatekeepers review the changes? Thanks,

-David Coakley / AMD Open Source Compiler Engineering

David Coakley

2012-02-27 02:23:22 UTC

Permalink

To answer a few questions:

Q: What is PROMPF?
A: It is a static analysis feature which generates an additional file
(.anl) during compilation. There is a lot of code in LNO to maintain
related information in WHIRL, but I don't think it is used to make any
changes during compilation. I believe the expectation was that
another tool would be used to interpret the analysis file... maybe
someone from the SGI days can clarify.

Q: Why does the patch remove the Whirl Browser?
A: It does not. It only removes the WB code related to PROMPF.

Q: Why not maintain or improve this functionality?
A: The patch is based on the assumption that no one is using this
feature because BUILD_SKIP_PROMPF has always been defined for all
targets in Open64. I tried to build a compiler with PROMPF support on
x86 and the feature didn't work, because no prompf_anl.so is created
and copied to the installation directory, further convincing me that
no one is using the feature.

Post by David Coakley
Hi all,
Here is a patch that removes dead code related to the obsolete PROMPF
feature. The patch is farily large but the changes are
straightforward. Removing this code makes the remaining code easier
to understand, especially for those new to Open64.
Could one of the gatekeepers review the changes? Thanks,
-David Coakley / AMD Open Source Compiler Engineering

C. Bergström

2012-02-27 08:30:41 UTC

Permalink

Post by David Coakley
Q: What is PROMPF?
A: It is a static analysis feature which generates an additional file
(.anl) during compilation. There is a lot of code in LNO to maintain
related information in WHIRL, but I don't think it is used to make any
changes during compilation. I believe the expectation was that
another tool would be used to interpret the analysis file... maybe
someone from the SGI days can clarify.

PathScale in the coming months may make an external tool which can read
the .anl files. I believe UH also had a project that could read it, but
I'm not sure the status. We're happy to work with the community on
improving this functionality and documenting any changes we make to the
.anl format

Post by David Coakley
Q: Why does the patch remove the Whirl Browser?
A: It does not. It only removes the WB code related to PROMPF.
Q: Why not maintain or improve this functionality?
A: The patch is based on the assumption that no one is using this
feature because BUILD_SKIP_PROMPF has always been defined for all
targets in Open64. I tried to build a compiler with PROMPF support on
x86 and the feature didn't work, because no prompf_anl.so is created
and copied to the installation directory, further convincing me that
no one is using the feature.

It should be fairly trivial to get this building.
-----------------------
TBH I'd support removal of this code since
1) I think this functionality can be improved and the current state may
not be the best place to start
2) Unless the Open64 community is willing to develop their own or
collaborate with others you're not guaranteed to be able to use the
external tools

David Coakley

2012-03-05 08:29:13 UTC

Permalink

Christopher, thanks for the input, I agree with your reasons for
removing the code.

Have any of the Open64 gatekeepers looked at this change yet?

PathScale in the coming months may make an external tool which can read the
.anl files. I believe UH also had a project that could read it, but I'm not
sure the status. We're happy to work with the community on improving this
functionality and documenting any changes we make to the .anl format

It should be fairly trivial to get this building.
-----------------------
TBH I'd support removal of this code since
1) I think this functionality can be improved and the current state may not
be the best place to start
2) Unless the Open64 community is willing to develop their own or
collaborate with others you're not guaranteed to be able to use the external
tools

Sun Chan

2012-03-05 08:34:24 UTC

Permalink

I did not review the changes since it's all about removing code, which
is always a good thing. As long as you run enough test (make sure
enough openMP test), I am fine with the check in
Sun

Post by David Coakley
Christopher, thanks for the input, I agree with your reasons for
removing the code.
Have any of the Open64 gatekeepers looked at this change yet?

PathScale in the coming months may make an external tool which can read the
.anl files. I believe UH also had a project that could read it, but I'm not
sure the status. We're happy to work with the community on improving this
functionality and documenting any changes we make to the .anl format

It should be fairly trivial to get this building.
-----------------------
TBH I'd support removal of this code since
1) I think this functionality can be improved and the current state may not
be the best place to start
2) Unless the Open64 community is willing to develop their own or
collaborate with others you're not guaranteed to be able to use the external
tools

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Open64-devel mailing list
https://lists.sourceforge.net/lists/listinfo/open64-devel

David Coakley

2012-03-07 04:10:43 UTC

Permalink

Thanks, Sun. I tested extensively, including running SPEC CPU2006
with -apo and did not see any changes.

I'll commit the change in a few days if there are no objections.

Post by Sun Chan
I did not review the changes since it's all about removing code, which
is always a good thing. As long as you run enough test (make sure
enough openMP test), I am fine with the check in
Sun

Post by David Coakley
Christopher, thanks for the input, I agree with your reasons for
removing the code.
Have any of the Open64 gatekeepers looked at this change yet?

PathScale in the coming months may make an external tool which can read the
.anl files. I believe UH also had a project that could read it, but I'm not
sure the status. We're happy to work with the community on improving this
functionality and documenting any changes we make to the .anl format

It should be fairly trivial to get this building.
-----------------------
TBH I'd support removal of this code since
1) I think this functionality can be improved and the current state may not
be the best place to start
2) Unless the Open64 community is willing to develop their own or
collaborate with others you're not guaranteed to be able to use the external
tools

Christophe MONAT

2012-09-27 09:24:49 UTC

Permalink

David,

Was that patch ever applied in the code base ?

--C

David Coakley

2012-09-28 04:58:44 UTC

Permalink

Yes, it's r3876 in the trunk.

-David

On Thu, Sep 27, 2012 at 2:24 AM, Christophe MONAT

Post by Christophe MONAT
David,
Was that patch ever applied in the code base ?
--C