Explicitly specify target in Compilation constructor #5506
Conversation
|
Tagging @harryyu1994 @mpirvu @dsouzai for review. |
dchopra001
force-pushed
the
cpuDetection
branch
from
48f80e8 to
4de70eb
Compare
|
The following tests: Failed with what look like port library tests. I don't think these are related. |
| } | ||
| else | ||
| { | ||
| _target = TR::Compiler->target; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
I was suggesting to not attach a default value to _target and force user to specify one when they use TR::Compilation().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually; don't you need to also add the change to the constructor of TR::Compilation? ie in https://github.com/eclipse/omr/blob/master/compiler/compile/Compilation.hpp
This is needed so that the codegen is able to set the correct flags when initializing. Signed-off-by: Dhruv Chopra <[email protected]>
dchopra001
force-pushed
the
cpuDetection
branch
from
4de70eb to
d0e0cc1
Compare
I have made the change here: d0e0cc1 |
|
given #4518 is open I'm ok with merging this now, but I think that issue really needs some attention - this is a bug waiting to happen. |
|
Leaving this open until Monday if anyone else has concerns given the flurry of discussion above. @genie-omr build all |
|
@fjeremic: Please don't merge this yet. I'd like to review and think through a few things, hopefully by end of day. |
Sure. I'll transfer committer over to you then. |
There was a problem hiding this comment.
I think the changes proposed here are fine. I agree that the larger problem of a disparity between the environment set outside of a compilation context (i.e., via TR::Compiler->target.cpu) versus the target within a compilation context (i.e., via comp()->target) is not addressed, but this makes the current solution incrementally better because it introduces the target before the CodeGenerator object is allocated.
However, despite #4518 remaining open it is not immediately clear without reading through the issue and between the lines what the next steps are to resolve this properly. That's not a discussion for this PR, however. I will pick up the discussion on that topic in #4518.
This is needed so that the codegen is able to set the correct
flags when initializing.
Signed-off-by: Dhruv Chopra [email protected]