Add optimization bisect opt-in calls for NVPTX passes

[andrew.w.kaylor]

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.

[jlebar]

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]

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.

[jlebar]

In D19518#412381, @jingyue wrote:

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]

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 ↗

(On Diff #54957)

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

[andrew.w.kaylor]

In D19518#412434, @jingyue wrote:

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.

[andrew.w.kaylor]

lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp

196 ↗

(On Diff #54957)

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]

Removed skip check for NVPTXLowerKernelArgs

[jingyue]

In D19518#412546, @andrew.w.kaylor wrote:

In D19518#412434, @jingyue wrote:

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 ↗

(On Diff #54957)

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

llvm/trunk/lib/Target/NVPTX/NVPTXFavorNonGenericAddrSpaces.cpp

bool NVPTXFavorNonGenericAddrSpaces::optimizeMemoryInstruction(Instruction *MI,
   // Note: the addrspacecast can also be a constant expression.
   assert(isEliminableAddrSpaceCast(NewOperand));
   Operator *ASC = dyn_cast<Operator>(NewOperand);
   MI->setOperand(Idx, ASC->getOperand(0));
   return true;
 }
 
 bool NVPTXFavorNonGenericAddrSpaces::runOnFunction(Function &F) {
-  if (DisableFavorNonGeneric)
+  if (DisableFavorNonGeneric || skipFunction(F))
     return false;
 
   bool Changed = false;
   for (BasicBlock &B : F) {
     for (Instruction &I : B) {
       if (isa<LoadInst>(I)) {
         // V = load P
         Changed |= optimizeMemoryInstruction(&I, 0);

llvm/trunk/lib/Target/NVPTX/NVPTXImageOptimizer.cpp

 }
 
 char NVPTXImageOptimizer::ID = 0;
 
 NVPTXImageOptimizer::NVPTXImageOptimizer()
   : FunctionPass(ID) {}
 
 bool NVPTXImageOptimizer::runOnFunction(Function &F) {
+  if (skipFunction(F))
+    return false;
+
   bool Changed = false;
   InstrToDelete.clear();
 
   // Look for call instructions in the function
   for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;
        ++BI) {
     for (BasicBlock::iterator I = (*BI).begin(), E = (*BI).end();
          I != E; ++I) {

llvm/trunk/lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp

static unsigned joinAddressSpaces(unsigned AS1, unsigned AS2) {
   if (AS2 == ADDRESS_SPACE_UNINITIALIZED)
     return AS1;
 
   // The join of two different specific address spaces is generic.
   return AS1 == AS2 ? AS1 : (unsigned)AddressSpace::ADDRESS_SPACE_GENERIC;
 }
 
 bool NVPTXInferAddressSpaces::runOnFunction(Function &F) {
+  if (skipFunction(F))
+    return false;
+
   // Collects all generic address expressions in postorder.
   std::vector<Value *> Postorder = collectGenericAddressExpressions(F);
 
   // Runs a data-flow analysis to refine the address spaces of every expression
   // in Postorder.
   ValueToAddrSpaceMapTy InferredAddrSpace;
   inferAddressSpaces(Postorder, &InferredAddrSpace);
 

llvm/trunk/lib/Target/NVPTX/NVPTXLowerAlloca.cpp

 
 INITIALIZE_PASS(NVPTXLowerAlloca, "nvptx-lower-alloca",
                 "Lower Alloca", false, false)
 
 // =============================================================================
 // Main function for this pass.
 // =============================================================================
 bool NVPTXLowerAlloca::runOnBasicBlock(BasicBlock &BB) {
+  if (skipBasicBlock(BB))
+    return false;
+
   bool Changed = false;
   for (auto &I : BB) {
     if (auto allocaInst = dyn_cast<AllocaInst>(&I)) {
       Changed = true;
       auto PTy = dyn_cast<PointerType>(allocaInst->getType());
       auto ETy = PTy->getElementType();
       auto LocalAddrTy = PointerType::get(ETy, ADDRESS_SPACE_LOCAL);
       auto NewASCToLocal = new AddrSpaceCastInst(allocaInst, LocalAddrTy, "");

llvm/trunk/lib/Target/NVPTX/NVPTXPeephole.cpp

static void CombineCVTAToLocal(MachineInstr &Root) {
   // Check if MRI has only one non dbg use, which is Root
   if (MRI.hasOneNonDBGUse(Prev.getOperand(0).getReg())) {
     Prev.eraseFromParentAndMarkDBGValuesForRemoval();
   }
   Root.eraseFromParentAndMarkDBGValuesForRemoval();
 }
 
 bool NVPTXPeephole::runOnMachineFunction(MachineFunction &MF) {
+  if (skipFunction(*MF.getFunction()))
+    return false;
+
   bool Changed = false;
   // Loop over all of the basic blocks.
   for (auto &MBB : MF) {
     // Traverse the basic block.
     auto BlockIter = MBB.begin();
 
     while (BlockIter != MBB.end()) {
       auto &MI = *BlockIter++;