Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly specify target in Compilation constructor #5506

Merged
merged 1 commit into from
Sep 1, 2020
Merged

Explicitly specify target in Compilation constructor #5506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions compiler/compile/Compilation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class OMR_EXTENSIBLE Compilation : public OMR::CompilationConnector
TR::Options &options,
TR::Region &heapMemoryRegion,
TR_Memory *memory,
TR_OptimizationPlan *optimizationPlan) :
TR_OptimizationPlan *optimizationPlan,
TR::Environment *target = NULL) :
OMR::CompilationConnector(
compThreadId,
omrVMThread,
Expand All @@ -59,7 +60,8 @@ class OMR_EXTENSIBLE Compilation : public OMR::CompilationConnector
options,
heapMemoryRegion,
memory,
optimizationPlan)
optimizationPlan,
target)
{}

~Compilation() {}
Expand Down
13 changes: 11 additions & 2 deletions compiler/compile/OMRCompilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ OMR::Compilation::Compilation(
TR::Options &options,
TR::Region &heapMemoryRegion,
TR_Memory *m,
TR_OptimizationPlan *optimizationPlan) :
TR_OptimizationPlan *optimizationPlan,
TR::Environment *target) :
_signature(compilee->signature(m)),
_options(&options),
_heapMemoryRegion(heapMemoryRegion),
Expand Down Expand Up @@ -294,9 +295,17 @@ OMR::Compilation::Compilation(
_gpuPtxCount(0),
_bitVectorPool(self()),
_typeLayoutMap((LayoutComparator()), LayoutAllocator(self()->region())),
_target(TR::Compiler->target),
_tlsManager(*self())
{
if (target != NULL)
dchopra001 marked this conversation as resolved.
Show resolved Hide resolved
{
_target = *target;
}
else
{
_target = TR::Compiler->target;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the pattern to be adopted does the visibility of TR::Compiler->target need to change to stop people accidentally using it rather than the target from the compilation? Having the two and having them potentially vary seems dangerous and likely to produce subtle bugs...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting to not attach a default value to _target and force user to specify one when they use TR::Compilation().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @andrewcraik's concern is around other areas of the compiler codebase using TR::Compiler-> such as the optimizer or codegen and expecting it to be the same as comp()->target()->. Does that sound correct?

Whether we specify a _target or not in TR::Compilation(), his concern will still be valid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dchopra001 yup - any time there are two ways to answer seemingly the same question and they don't give the same answer you will get bugs. So I would prefer we find a way to avoid this duality or prevent the bugs before they happen through the structure of the code if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a problem before as well though. We would change the comp()->target's cpu inside the TR::Compilation constructor. So if anyone was using TR::Compiler->target.cpu to grab information, it would end up being incorrect. I don't think the code in this PR specifically is introducing this problem.

Copy link
Contributor

@harryyu1994 harryyu1994 Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, that seems like a tough issue to solve and we've already run into countless bugs related to comp()->target() vs. TR::Compiler->target. In places where we are not doing compilation, TR::Compiler->target is still needed.

Copy link
Contributor

@dsouzai dsouzai Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the pattern to be adopted does the visibility of TR::Compiler->target need to change to stop people accidentally using it rather than the target from the compilation? Having the two and having them potentially vary seems dangerous and likely to produce subtle bugs...

Most of the queries to the target is done via the comp object; there are some outstanding locations where that hasn't happened (see #4518). I think my intention originally was to make the _target private to TR::Compiler but I couldn't do it because of those outstanding locations.

EDIT: Here's the PR where the switch to comp local target was made: #4568

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe changes were made as part of the processor detection work to avoid using TR::Compiler->target.cpu.getSupportsArch and only use comp()->target().cpu.isAtLeast when checking for a particular architecture level support. I believe this is the only area that would be different at the moment.

But I agree, this is an issue, and I did have to deal with the above mentioned problem on Z.

}

//Avoid expensive initialization and uneeded option checking if we are doing AOT Loads
if (_optimizationPlan && _optimizationPlan->getIsAotLoad())
{
Expand Down
3 changes: 2 additions & 1 deletion compiler/compile/OMRCompilation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,8 @@ class OMR_EXTENSIBLE Compilation
TR::Options &,
TR::Region &heapMemoryRegion,
TR_Memory *,
TR_OptimizationPlan *optimizationPlan
TR_OptimizationPlan *optimizationPlan,
TR::Environment *target = NULL
);

TR::SymbolReference * getSymbolReferenceByReferenceNumber(int32_t referenceNumber);
Expand Down