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.

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

Compiler FE: Remove optimization "--all" #5780

Closed
seanshpark opened this issue Jan 21, 2021 · 9 comments
Closed

Compiler FE: Remove optimization "--all" #5780

seanshpark opened this issue Jan 21, 2021 · 9 comments

Comments

@seanshpark
Copy link
Contributor

seanshpark commented Jan 21, 2021

The --all option was first introduced to enable all the optimization options but currently it didn't turned out that way.
To add new option to the all maybe risky in terms of stability.
As of result, all misleads that one may expect it will do all optimizations but actually it doesn't.

I think it would be better to remove this all option and provide some more specific group of options as an alias.

I'd like to know if there are any problems before just removing this option.
If there is no comments about a week, I'll go on removing them.

@seanshpark
Copy link
Contributor Author

CC @samsung/ONE

@lemmaa
Copy link
Member

lemmaa commented Jan 22, 2021

As a suggestion, what if we provide the -O1, -O2, and -O3 options like c compiler?

As you might expect, it's kind of a group of optimization options. Probably the most stable optimizations are reflected in -O1, the slightly challenging ones in -O2, and the risky, perhaps current --all items are reflected through -O3. Of course, the configuration of the options in this group can be changed at any time at our discretion, so users can use it without confusion. The long format of this option would be around --optimization-level={1,2,3}.

In addition, options such as -O4 or --experimental could be added.

@seanshpark
Copy link
Contributor Author

As a suggestion, what if we provide the -O1, -O2, and -O3 options like c compiler?

Thanks! I'll add another issue for this, as this issue is just about removing --all option.

@seanshpark
Copy link
Contributor Author

Where --all is used, for testing

  • compiler/common-artifacts/CMakeLists.txt
  if(NOT DEFINED NO_OPTIMIZE_${RECIPE})
    # Generate optimized .circle
    add_custom_command(OUTPUT ${OPT_CIRCLE_OUTPUT_PATH}
      COMMAND $<TARGET_FILE:circle2circle> --all ${CIRCLE_OUTPUT_PATH} ${OPT_CIRCLE_OUTPUT_PATH}
      DEPENDS $<TARGET_FILE:circle2circle>  ${CIRCLE_OUTPUT_PATH}
      COMMENT "Generate ${OPT_CIRCLE_FILE}"
    )
    set(OPT_FORMAT ".opt")
    list(APPEND TEST_DEPS ${OPT_CIRCLE_OUTPUT_PATH})
  endif()
  • docs/compiler/compiler/common-artifacts/CMakeLists.txt
  if(NOT DEFINED NO_OPTIMIZE_${RECIPE})
    # Generate optimized .circle
    add_custom_command(OUTPUT ${OPT_CIRCLE_OUTPUT_PATH}
      COMMAND $<TARGET_FILE:circle2circle> --all ${CIRCLE_OUTPUT_PATH} ${OPT_CIRCLE_OUTPUT_PATH}
      DEPENDS $<TARGET_FILE:circle2circle>  ${CIRCLE_OUTPUT_PATH}
      COMMENT "Generate ${OPT_CIRCLE_FILE}"
    )
    set(OPT_FORMAT ".opt")
    list(APPEND TEST_DEPS ${OPT_CIRCLE_OUTPUT_PATH})
  endif()
  • and compiler/one-cmds/tests/one-optimize_001.test and several one-optimize tests

@seanshpark
Copy link
Contributor Author

seanshpark commented Jan 27, 2021

Current --all options

  if (arser.get<bool>("--all"))
  {
    options->enable(Algorithms::FuseBCQ);
    options->enable(Algorithms::FuseInstanceNorm);
    options->enable(Algorithms::ResolveCustomOpAdd);
    options->enable(Algorithms::ResolveCustomOpBatchMatMul);
    options->enable(Algorithms::ResolveCustomOpMatMul);
    options->enable(Algorithms::RemoveRedundantTranspose);
    options->enable(Algorithms::SubstitutePackToReshape);
  }

For backward compatibility with the test, I want to give an name to replace --all ... but the can't find an proper name for this... there seems no relation for each of them.

Another thought is to refactor dredd tests... problem is one .rule file is shared in several tests, and it would be better to do this before things get huge.

@mhs4670go
Copy link
Contributor

@seanshpark How about renaming --all with the name from #5784?(maybe O1?) I mean decide #5784 first and then apply it to this issue.

@seanshpark
Copy link
Contributor Author

How about renaming --all with the name from #5784?(maybe O1?) I mean decide #5784 first and then apply it to this issue.

That is one good suggestion.
One strange thing about current --all is that I can't explain why these options are grouped to --all or -O1.
Only I could say is "it's... historical reason..."

@seanshpark
Copy link
Contributor Author

seanshpark commented Feb 1, 2021

I can't find any good name for current --all.
As suggestion, I'll use --experimental for it.
--> I think experimental is for grouo options for experiments... --O1 would be better I think...

@seanshpark
Copy link
Contributor Author

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants