arcana-lab / noelle

NOELLE Offers Empowering LLVM Extensions
MIT License
75 stars 36 forks source link

Repl extensible change #68

Closed vgene closed 1 year ago

vgene commented 1 year ago

I made the following changes:

  1. Expose SCAF AA pointer for the repl to understand which analysis contributes to what.
  2. Repl changes to be extensible
  3. Automatic formatting change of src/core/loop_scc_attributes/src/StackObjectClonableSCC.cpp
scampanoni commented 1 year ago

There are two problems with the change added to LoopDependenceInfo.hpp.

1) the API "void getSCAFLoopAA()" breaks now the abstraction of dependence graphs that hidden the set of analyses that power them. If this is needed, then we should re-think about the abstraction rather than creating a "leaking abstraction" like this one. 2) if the API void getSCAFLoopAA(); does not take any parameter, we should put "void" in the parameter list so we get a compilation error if we accidentally pass parameters to a call to it.

vgene commented 1 year ago

Addressed point 2. For point 1, I understand it breaks the abstraction. This leaky abstraction is a hack required for cpf-repl to play with the SCAF analysis chain in order to get the set of analyses that contribute to a query. I can't think of another use for this abstraction. We can mark in the comment that this is not for normal use. It'll be great if there is a better way to get the LoopAA pointer that doesn't break the abstraction.

vgene commented 1 year ago

@scampanoni Maybe one idea to make the situation better: we can wrap this function in a different namespace (something very explicit like NOELLE::LeakyAbstraction), so it won't be mistaken for other uses while we try to find a better solution to replace it.

tommymcm commented 1 year ago

If it's a hack required for cpf-repl then why does it need to be merged into the main branch instead of maintained in its own branch or fork?

vgene commented 1 year ago

Addressed @tommymcm 's point. I reverted the changed files for this PR.

vgene commented 1 year ago

This should be ready for merge @scampanoni