This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Disable the scalar IR, SDWA and load store vectorizer passes at -O1
ClosedPublic

Authored by bsaleil on Apr 27 2021, 4:55 PM.

Details

Summary

This patch disables some of the passes at -O1. These passes have a significant impact on compilation time, so we only want them to be enabled starting from -O2.

Diff Detail

Event Timeline

bsaleil created this revision.Apr 27 2021, 4:55 PM
bsaleil requested review of this revision.Apr 27 2021, 4:55 PM

Needs a testcase. I think we have some pass pipeline tests already to show what's run (maybe not for -O1, I didn't know anyone actually used it)

bsaleil updated this revision to Diff 341265.Apr 28 2021, 10:56 AM

Add test case

Needs a testcase. I think we have some pass pipeline tests already to show what's run (maybe not for -O1, I didn't know anyone actually used it)

@arsenm, I think we only had a test case for opt, so I added a new test case to show which passes are run for llc for all the optimization levels.

foad added inline comments.Apr 29 2021, 2:34 AM
llvm/test/CodeGen/AMDGPU/llc-pipeline.ll
9

I would suggest not checking the "Pass Arguments:" line at all here, since any diffs to this very long line will be very hard to read anyway. All the information should be contained in the lines below.

bsaleil updated this revision to Diff 341619.Apr 29 2021, 1:09 PM

Remove checks for the "Pass Arguments:" lines in the test case.

bsaleil marked an inline comment as done.Apr 29 2021, 1:09 PM
arsenm added inline comments.Apr 29 2021, 3:15 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
903–905

This seems like a weird interaction between flags. Maybe this should check if EnableScalarIRPasses was explicitly used? Or maybe we can just drop EnableScalarIRPasses entirely

foad added inline comments.Apr 30 2021, 1:05 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
903–905

I agree that EnableScalarIRPasses.getNumOccurrences() ? EnableScalarIRPasses : TM.getOptLevel() > CodeGenOpt::Less would be saner.

bsaleil updated this revision to Diff 342017.Apr 30 2021, 1:22 PM

Enable the passes only from -O2 or when explicitly enabled with flags. Also update the test case to ensure the passes are run for -O1 when explicitly enabled.

bsaleil marked 2 inline comments as done.Apr 30 2021, 1:23 PM
bsaleil added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
903–905

You're right, that makes a lot more sense. I just updated the patch and added a RUN line in the test case to ensure the passes are run for -O1 when we explicitly provide the flags.

bsaleil marked an inline comment as done.May 3 2021, 7:39 AM
arsenm accepted this revision.May 3 2021, 2:18 PM
This revision is now accepted and ready to land.May 3 2021, 2:18 PM
This revision was landed with ongoing or failed builds.May 4 2021, 1:45 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 4 2021, 2:47 PM

This breaks incremental builders: http://45.33.8.238/linux/45830/step_12.txt

Ptal. You'll need a -o /dev/null and an rm -f for the temp file to clean up bots (can remove the latter after a bit)

This breaks incremental builders: http://45.33.8.238/linux/45830/step_12.txt

Ptal. You'll need a -o /dev/null and an rm -f for the temp file to clean up bots (can remove the latter after a bit)

@thakis I already committed a fix to avoid generating the .s file. Should I add the rm -f directly into the RUN lines ? Is something like RUN: rm %S/llc-pipeline.s -f ok ?

thakis added a comment.May 4 2021, 3:23 PM

This breaks incremental builders: http://45.33.8.238/linux/45830/step_12.txt

Ptal. You'll need a -o /dev/null and an rm -f for the temp file to clean up bots (can remove the latter after a bit)

@thakis I already committed a fix to avoid generating the .s file. Should I add the rm -f directly into the RUN lines ? Is something like RUN: rm %S/llc-pipeline.s -f ok ?

Yes, that sounds perfect. See e.g. clang/test/CoverageMapping/coroutine.cpp for an example :)

critson added a subscriber: critson.May 5 2021, 4:08 AM

llc-pipeline.ll has problems with -DLLVM_ENABLE_EXPENSIVE_CHECKS=1 as instruction verification seems to still take place.

foad added a comment.May 5 2021, 4:21 AM

llc-pipeline.ll has problems with -DLLVM_ENABLE_EXPENSIVE_CHECKS=1 as instruction verification seems to still take place.

test/CodeGen/X86/opt-pipeline.ll uses grep -v to filter these lines out.

llc-pipeline.ll has problems with -DLLVM_ENABLE_EXPENSIVE_CHECKS=1 as instruction verification seems to still take place.

test/CodeGen/X86/opt-pipeline.ll uses grep -v to filter these lines out.

Thanks, I'll update the test to filter the lines with grep -v too for these bots.