This is an archive of the discontinued LLVM Phabricator instance.

Add optimization bisect opt-in calls for NVPTX passes
ClosedPublic

Authored by andrew.w.kaylor on Apr 25 2016, 6:09 PM.

Details

Summary

This patch adds calls to NVPTX-specific passes that can be safely skipped to opt-in to the optimization bisect mechanism.

I selected the passes to be skipped based on the fact that they were not added at CodeGenOpt::None in NVPTXTargetMachine.cpp. Based on that criteria, I did not add opt-in calls to the following passes, which appear to be required:

GenericToNVVM
NVPTXAllocaHoisting
NVPTXAssignValidGlobalNames
NVPTXLowerAggrCopies
NVPTXPeephole
NVPTXPrologEpilogPass
NVVMReflect
NVPTXReplaceImageHandles

Based on the description (not to mention the name) NVPTXPeephole seemed to me as if it could be safely skipped but it appears that it never is, so I did not add the opt-in call there.

Note that the call to skipFunction() will also check for the "optnone" function attribute, so this can theoretically result in passes being skipped even when optimization bisect is not being done. However, I believe that any pass that can be safely skipped should be skipped for functions with the optnone attribute.

See D19172 for details on the base optimizaton bisect implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

andrew.w.kaylor retitled this revision from to Add optimization bisect opt-in calls for NVPTX passes.
andrew.w.kaylor updated this object.
andrew.w.kaylor added a reviewer: jholewinski.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: llvm-commits.
jlebar added a subscriber: jlebar.Apr 25 2016, 6:20 PM

I did not add opt-in calls to the following passes, which appear to be required:

Those all seem required to me, with the exception of the peephole optimizer, like you say.

I think the changes you've made here are safe, but I'm not super-familiar with these passes, so I'd prefer if someone else could double-check.

jingyue edited edge metadata.Apr 26 2016, 10:35 AM

Hi Andrew,

What do you mean by "NVPTXPeephole can't be safely skipped"? I tried disabling it and then running all tests under test/CodeGen/NVPTX. None of the runs crash, and only local-stack-frame.ll failed because it verifies NVPTXPeephole indeed optimizes away redundant stack frame operations.

Hi Andrew,

What do you mean by "NVPTXPeephole can't be safely skipped"? I tried disabling it and then running all tests under test/CodeGen/NVPTX. None of the runs crash, and only local-stack-frame.ll failed because it verifies NVPTXPeephole indeed optimizes away redundant stack frame operations.

Based on the description (not to mention the name) NVPTXPeephole seemed to me as if it could be safely skipped but it appears that it never is, so I did not add the opt-in call there.

(Emphasis mine.) The issue AIUI is that NVPTXPeephole is enabled at -O0, so Andrew was conservatively assuming it was this way for a reason.

jingyue requested changes to this revision.Apr 26 2016, 11:00 AM
jingyue edited edge metadata.

Oh, I see what you mean now.

So NVPTXPeephole should be guarded by CodeGenOpt::None and thus is safe to skip.

NVPTXLowerKernelArgs is unsafe to skip. See my inlined comments.

lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp
196

handleByValParam is necessary for correctness, so NVPTXLowerKernelArgs can't be skipped unless we separate handleByValParam out to another pass.

This revision now requires changes to proceed.Apr 26 2016, 11:00 AM

Oh, I see what you mean now.

So NVPTXPeephole should be guarded by CodeGenOpt::None and thus is safe to skip.

The optnone-llc.ll test verifies that nothing is skipped which is run at -O0, so as long as this pass is being run at -O0 we can't add the call to skip it for "optnone" functions and bisection. If the target machine code were updated to skip the peephole pass at -O0 then the skip check could be added.

lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp
196

As far as I can tell, this pass is only ever added by NVPTXPassConfig::addAddressSpaceInferencePasses(), which is not called when the opt level is CodeGenOpt::None.

So if this pass is necessary there may be another problem that needs to be addressed regarding when and where the pass gets added.

I'll take the skip check out, but that's something you may want to look into.

andrew.w.kaylor edited edge metadata.

Removed skip check for NVPTXLowerKernelArgs

jingyue accepted this revision.Apr 26 2016, 4:08 PM
jingyue edited edge metadata.

Oh, I see what you mean now.

So NVPTXPeephole should be guarded by CodeGenOpt::None and thus is safe to skip.

The optnone-llc.ll test verifies that nothing is skipped which is run at -O0, so as long as this pass is being run at -O0 we can't add the call to skip it for "optnone" functions and bisection. If the target machine code were updated to skip the peephole pass at -O0 then the skip check could be added.

In rL267619, I disabled NVPTXPeephole when CodeGenOpt::None. Can you add skipFunction in this patch accordingly?

lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp
196

Thanks for pointing this out. I fixed this issue in rL267619.

This revision is now accepted and ready to land.Apr 26 2016, 4:08 PM
This revision was automatically updated to reflect the committed changes.