[luci] Introduce the parent class method to luci::Pass #14594

tomdol · 2025-01-28T14:42:53Z

This directive fixes a build break caused by a compiler error emitted when compiling with -Werror=overloaded-virtual flag switched on. The error is about the logo::Pass::run method being hidden by luci::Pass::run.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak [email protected]

Related issue NPU_Compiler/issues/19901

This directive fixes a build break caused by a compiler error emitted when compiling with -Werror=overloaded-virtual flag switched on. The error is about the logo::Pass::run method being hidden by luci::Pass::run. ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <[email protected]>

This directive fixes a build break caused by a compiler error emitted when compiling with -Werror=overloaded-virtual flag switched on. The error is about the logo::Pass::run method being hidden by luci::Pass::run. This is a backport of Samsung#14594 to the release branch. ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <[email protected]>

seanshpark · 2025-02-02T22:45:19Z

@lemmaa , what is the guide for putting internal server URLs?

seanshpark · 2025-02-02T22:48:54Z

compiler/luci/pass/include/luci/ModulePass.h

@@ -28,6 +28,8 @@ namespace luci
 class Pass : public logo::Pass
 {
 public:
+  // This directive prevents emitting a compiler erro triggered by -Werror=overloaded-virtual
+  using logo::Pass::run;


I don't understand why add this here.
I cannot accept the resolution method. please find another way of solution.

This is in fact the solution. The reason this error occurs in the first place is that the luci::Pass::run(luci::Module *) has a different signature than the parent's loco::Pass::run(loco::Graph *) and this can cause overload resolution problems when compiling the code in certain situations. This is why newer compilers report a warning and the -Werror=overloaded-virtual makes it an error which basically blocks the build on Ubuntu 24.

The using directive adds the logo::Pass::run method to the overload set of the luci::Pass class which makes the warning(error) go away and most importantly - it doesn't break anything.

IMHO you got it wrong. luci::Pass::run(luci::Module *) is also pure virtual so that inherit class SHOULD also override.
You can inherit from loco::Pass if do not want to use luci::Module * parameter.

The original build error (screenshot in the linked issue) comes from a file CircleNodePass.h which includes the logo/Pass.h header and contains a class which inherits from logo::Pass. This is correct and matches what you said above - CircleNodePass overrides run(loco::Graph* graph) and doesn't touch the version which operates on luci::Module.

Yet the compiler produces an error about luci::Pass::run hiding logo::Pass::run withing this compilation context.

Here's a smaller snippet that demonstrates how hiding the virtual methods interferes with name lookup and overload resolution https://ideone.com/lI3QNL

Uncomment the using A::run; line fixes the problem and this is exactly what I'm proposing here.

The only other solution I can imagine is renaming the conflicting methods so that they don't hide each other:

logo::Pass::run(Graph*) -> logo::Pass::run_on_graph(Graph*)
luci::Pass::run(Module*) -> luci::Pass::run_on_module(Module*)

First of all, I'm not familiar with this kind of warning and I didn't understand neither what above code tries to solve at the first.
So, I've searched the general solution for this warning and found that most of directions guided as @tomdol's.

"In cases where the different signatures are not an accident, the simplest solution is to add a using-declaration to the derived class to un-hide the base function, e.g. add using A::f; to B. "

from GCC Docs : -Woverloaded-virtual

We may solve the issue in different ways (e.g. duplicating logo::pass to luci:pass, renaming the functions ...) but current suggestion seems quite optimal for me ;)

Have you tried overriding luci::Pass::run(luci::Module *) ? something like

bool run(luci::Module *) override { throw "NYI"; }

It's impossible to override the suggested method because it's a member of luci::Pass. The NPU_Compiler does not inherit from this class. It uses logo::Pass instead.

The reason this became a problem is the change how GCC13 and newer versions compile the whole code base. The inheritance hierarchy of logo::Pass <- luci::Pass is analyzed independently from logo::Pass <- CircleNodePass so no matter what I do this warning will still be reported.

I don't understand why add this here. I cannot accept the resolution method. please find another way of solution.

@seanshpark , I believe some positive opinions about this resolution method were added:
#14594 (comment)
#14594 (comment)

and I believe this resolution is based on GCC documentation. So please give us your final decision about this PR, we want to merge it or close it and prepare patch for ONE in our project to solve this issue on our side.

I cannot accept the resolution method. please find another way of solution.

There was no updates after this comment.
I still do not accept this change.

The reason this became a problem is the change how GCC13 and newer versions compile the whole code base. The inheritance hierarchy of logo::Pass <- luci::Pass is analyzed independently from logo::Pass <- CircleNodePass so no matter what I do this warning will still be reported.

#14594 (comment)

There's no other way of solving this warning but the using directive.

seanshpark · 2025-02-03T21:41:10Z

I assume this issue is to fix for U24.04 cause of your situation.
Current ONE/compiler does not have CI for U24.04.
I cannot confirm this check is appropriate for the issue in internal compiler.
Personally I do not want this patch to land.

What I can suggest is written in #14589 (comment)

dahlinPL · 2025-02-11T14:00:50Z

Please take a look into #14448, I believe this workaround can be removed after fixing it in a way presented in this PR, @hseok-oh, you're author of #14448 what's your opinion?

hseok-oh · 2025-02-12T06:07:47Z

@dahlinPL @seanshpark We can see this warning when we are using Clang or latest GCC version.

https://stackoverflow.com/questions/18515183/c-overloaded-virtual-function-warning-by-clang

This warning is there to prevent accidental hiding of overloads when overriding is intended.

Clang users have been experiencing this issue for more than 10 years. And now GCC has begun to warn this issue, unfortunately.
I added comment that TODO Remove -Wno-reoder and -Wno-overloaded-virtual on #14448. But I've changed my opinion because I think overloaded-virtual warning is excessive. I want to ignore this warning by keeping flag -Wno-overloaded-virtual.

dahlinPL · 2025-02-13T13:54:07Z

@dahlinPL @seanshpark We can see this warning when we are using Clang or latest GCC version.

https://stackoverflow.com/questions/18515183/c-overloaded-virtual-function-warning-by-clang

This warning is there to prevent accidental hiding of overloads when overriding is intended.

Clang users have been experiencing this issue for more than 10 years. And now GCC has begun to warn this issue, unfortunately. I added comment that TODO Remove -Wno-reoder and -Wno-overloaded-virtual on #14448. But I've changed my opinion because I think overloaded-virtual warning is excessive. I want to ignore this warning by keeping flag -Wno-overloaded-virtual.

I fully understand your concern :)

But please keep in mind below:

ONE can be used as dependency in some projects, and these projects can face this build issue and maybe project owners don't want to use -Wno-overloaded-virtual
in fact overloaded-virtual seems to be quite easy to fix in ONE, even if it's a bit excessive

so, even if opinions about overloaded-virtual usefulness vary, please reconsider fixing these few lines with explicity using to supress compiler doubts :)

I would be grateful, it will help us a lot

dahlinPL · 2025-03-13T07:45:27Z

@seanshpark @jinevening thanks for merging #14808, thanks to this we are one step closer to bump ONE version in our project. WIll you please reconsider also this contribution?

jinevening · 2025-03-14T00:38:53Z

First of all, I think this PR can be merged as-is. This seems to be a widely-used solution for this situation.

I can imagine another workaround that makes a separate class for bool run(luci::Module *) (ex: luci::ModulePass) and let luci::Pass inherit both classes (luci::ModulePass and logo::Pass). But I'm not sure the workaround is better than the current PR.

jinevening

LGTM

It's up to @seanshpark 's decision.

mbencer

LGTM

dahlinPL

LGTM

It's up to @seanshpark 's decision.

tomdol · 2025-03-21T08:48:56Z

Closing this PR since it hasn't been merged for almost 2 months despite providing the motivation and arguments explaining the proposed solution.

If anything changes, feel free to reopen.

tomdol force-pushed the overloaded_virtual branch from b3f60fe to 83e647c Compare January 28, 2025 14:59

tomdol mentioned this pull request Jan 28, 2025

[luci] Introduce the parent class method to luci::Pass #14595

Closed

seanshpark reviewed Feb 2, 2025

View reviewed changes

jinevening approved these changes Mar 14, 2025

View reviewed changes

mbencer approved these changes Mar 14, 2025

View reviewed changes

lemmaa assigned code-review-assistant and unassigned code-review-assistant Mar 17, 2025

dahlinPL approved these changes Mar 18, 2025

View reviewed changes

tomdol closed this Mar 21, 2025

tomdol mentioned this pull request Mar 25, 2025

[DRAFT] Rename the base interface of optimization passes #14907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[luci] Introduce the parent class method to luci::Pass #14594

[luci] Introduce the parent class method to luci::Pass #14594

tomdol commented Jan 28, 2025 •

edited by lemmaa

Loading

seanshpark commented Feb 2, 2025

seanshpark Feb 2, 2025 •

edited

Loading

tomdol Feb 3, 2025

seanshpark Feb 3, 2025

tomdol Feb 3, 2025

tomdol Feb 3, 2025

batcheu Feb 18, 2025

tomdol Mar 5, 2025

dahlinPL Mar 17, 2025

seanshpark Mar 17, 2025

tomdol Mar 17, 2025

seanshpark commented Feb 3, 2025

dahlinPL commented Feb 11, 2025

hseok-oh commented Feb 12, 2025 •

edited

Loading

dahlinPL commented Feb 13, 2025

dahlinPL commented Mar 13, 2025

jinevening commented Mar 14, 2025

jinevening left a comment

mbencer left a comment

dahlinPL left a comment

tomdol commented Mar 21, 2025

[luci] Introduce the parent class method to luci::Pass #14594

[luci] Introduce the parent class method to luci::Pass #14594

Conversation

tomdol commented Jan 28, 2025 • edited by lemmaa Loading

seanshpark commented Feb 2, 2025

seanshpark Feb 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanshpark commented Feb 3, 2025

dahlinPL commented Feb 11, 2025

hseok-oh commented Feb 12, 2025 • edited Loading

dahlinPL commented Feb 13, 2025

dahlinPL commented Mar 13, 2025

jinevening commented Mar 14, 2025

jinevening left a comment

Choose a reason for hiding this comment

mbencer left a comment

Choose a reason for hiding this comment

dahlinPL left a comment

Choose a reason for hiding this comment

tomdol commented Mar 21, 2025

tomdol commented Jan 28, 2025 •

edited by lemmaa

Loading

seanshpark Feb 2, 2025 •

edited

Loading

hseok-oh commented Feb 12, 2025 •

edited

Loading