[NVPTX] Check if callsite is defined when computing argument allignment

[jpienaar]

Summary

In getArgumentAlignment check if the ImmutableCallSite pointer CS is non-null before dereferencing. If CS is 0x0 fall back to the ABI type alignment else compute the alignment as before.

[jingyue]

test? :)

[vchuravy]

I have a potential test-case for this, but the patch doesn't apply cleanly to master so I was unable to test if this solves the problem.

strip.ll602 BDownload

[jpienaar]

Oh, sorry, I didn't see your response before I clicked abandoned. It has been a while, so this patch is pretty stale.

[vchuravy]

@jpienaar are you planning to work on this again? Or should I give it a go?

[vchuravy]

Rebases the original changes and adds a test-case

[vchuravy]

This is my first contribution to llvm so please let me know if I did something wrong in the process

[jpienaar]

Cool. I didn't know the review system allows having the patch updated like this :) It still reports me as the author and you as a subscriber. I don't think that matters.

lib/Target/NVPTX/NVPTXISelLowering.cpp

1051

There is a preference to early exits. So perhaps:

if (!CS)
  return DL.getABITTypeAlignment(Ty);

1132–1133

Move this to a new line to avoid exceeding 80 chars.

There are a couple of other formatting changes needed to. The simplest way is to use clang-format. To have the changes you added reformatted you could use clang-format-diff.py:
svn diff --diff-cmd=diff -x-U0 | ./tools/clang/tools/clang-format/clang-format-diff.py

test/CodeGen/NVPTX/zero-cs.ll

2

Could you add a comment explaining what this test is testing?

So this test case would fail previously (dereference null pointer) and now pass (return error)?

[vchuravy]

addresses review comments.

[vchuravy]

You don't own revision D9168: "[NVPTX] Check if callsite is defined when
computing argument allignment". Normally, you should only update
revisions you own. You can "Commandeer" this revision from the web
interface if you want to become the owner.

I was not able to figure out how to comandeer a revision, so i just went ahead and pushed it.

[jpienaar]

Looks good to me, do you need help submitting?

lib/Target/NVPTX/NVPTXISelLowering.cpp

1032

s/callsize/Call site/ ?

[vchuravy]

Fix spelling in comment

[vchuravy]

@jpienaar Since I don't have commit access, I think somebody else needs to commit this.

[jlebar]

FWIW I have run into this in the past and just not managed to muster up the energy to fix it. So, thank you!

[jlebar]

I was not able to figure out how to comandeer a revision, so i just went ahead and pushed it.

Under "leap into action", one of the options is to commandeer the revision.

lib/Target/NVPTX/NVPTXISelLowering.h

private:
 
   SDValue LowerSelect(SDValue Op, SelectionDAG &DAG) const;
 
   void ReplaceNodeResults(SDNode *N, SmallVectorImpl<SDValue> &Results,
                           SelectionDAG &DAG) const override;
   SDValue PerformDAGCombine(SDNode *N, DAGCombinerInfo &DCI) const override;
 
   unsigned getArgumentAlignment(SDValue Callee, const ImmutableCallSite *CS,
-                                Type *Ty, unsigned Idx) const;
+                                Type *Ty, unsigned Idx,
+                                const DataLayout &DL) const;
 };
 } // namespace llvm
 
 #endif

lib/Target/NVPTX/NVPTXISelLowering.cpp

for (unsigned i = 0, e = Args.size(); i != e; ++i, ++OIdx) {
     O << ".param .align " << align << " .b8 ";
     O << "_";
     O << "[" << sz << "]";
   }
   O << ");";
   return O.str();
 }
 
-unsigned
-NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
+unsigned NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
                                                    const ImmutableCallSite *CS,
-                                          Type *Ty,
-                                          unsigned Idx) const {
+                                                   Type *Ty, unsigned Idx,
+                                                   const DataLayout &DL) const {
+  if (!CS) {
+    // callsize is zero, fallback to ABI type alignment
+    return DL.getABITypeAlignment(Ty);
+  }
+
   unsigned Align = 0;
   const Value *DirectCallee = CS->getCalledFunction();
 
   if (!DirectCallee) {
     // We don't have a direct function symbol, but that may be because of
     // constant cast instructions in the call.
     const Instruction *CalleeI = CS->getInstruction();
     assert(CalleeI && "Call target is not a function or derived value?");
 
     // With bitcast'd call targets, the instruction will be the call
     if (isa<CallInst>(CalleeI)) {
       // Check if we have call alignment metadata
       if (llvm::getAlign(*cast<CallInst>(CalleeI), Idx, Align))
         return Align;
 
       const Value *CalleeV = cast<CallInst>(CalleeI)->getCalledValue();
       // Ignore any bitcast instructions
-      while(isa<ConstantExpr>(CalleeV)) {
+      while (isa<ConstantExpr>(CalleeV)) {
         const ConstantExpr *CE = cast<ConstantExpr>(CalleeV);
         if (!CE->isCast())
           break;
         // Look through the bitcast
         CalleeV = cast<ConstantExpr>(CalleeV)->getOperand(0);
       }
 
       // We have now looked past all of the bitcasts.  Do we finally have a
       // Function?
       if (isa<Function>(CalleeV))
         DirectCallee = CalleeV;
     }
   }
 
   // Check for function alignment information if we found that the
   // ultimate target is a Function
   if (DirectCallee)
     if (llvm::getAlign(*cast<Function>(DirectCallee), Idx, Align))
       return Align;
 
   // Call is indirect or alignment information is not available, fall back to
   // the ABI type alignment
-  auto &DL = CS->getCaller()->getParent()->getDataLayout();
   return DL.getABITypeAlignment(Ty);
 }
 
 SDValue NVPTXTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
                                        SmallVectorImpl<SDValue> &InVals) const {
   SelectionDAG &DAG = CLI.DAG;
   SDLoc dl = CLI.DL;
   SmallVectorImpl<ISD::OutputArg> &Outs = CLI.Outs;
for (unsigned i = 0, e = Args.size(); i != e; ++i, ++OIdx) {
     if (!Outs[OIdx].Flags.isByVal()) {
       if (Ty->isAggregateType()) {
         // aggregate
         SmallVector<EVT, 16> vtparts;
         SmallVector<uint64_t, 16> Offsets;
         ComputePTXValueVTs(*this, DAG.getDataLayout(), Ty, vtparts, &Offsets,
                            0);
 
-        unsigned align = getArgumentAlignment(Callee, CS, Ty, paramCount + 1);
+        unsigned align =
+            getArgumentAlignment(Callee, CS, Ty, paramCount + 1, DL);
         // declare .param .align <align> .b8 .param<n>[<size>];
         unsigned sz = DL.getTypeAllocSize(Ty);
         SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
         SDValue DeclareParamOps[] = { Chain, DAG.getConstant(align, dl,
                                                              MVT::i32),
                                       DAG.getConstant(paramCount, dl, MVT::i32),
                                       DAG.getConstant(sz, dl, MVT::i32),
                                       InFlag };
if (!Outs[OIdx].Flags.isByVal()) {
         }
         if (vtparts.size() > 0)
           --OIdx;
         ++paramCount;
         continue;
       }
       if (Ty->isVectorTy()) {
         EVT ObjectVT = getValueType(DL, Ty);
-        unsigned align = getArgumentAlignment(Callee, CS, Ty, paramCount + 1);
+        unsigned align =
+            getArgumentAlignment(Callee, CS, Ty, paramCount + 1, DL);
         // declare .param .align <align> .b8 .param<n>[<size>];
         unsigned sz = DL.getTypeAllocSize(Ty);
         SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
         SDValue DeclareParamOps[] = { Chain,
                                       DAG.getConstant(align, dl, MVT::i32),
                                       DAG.getConstant(paramCount, dl, MVT::i32),
                                       DAG.getConstant(sz, dl, MVT::i32),
                                       InFlag };
if (retTy->isFloatingPointTy() || retTy->isIntegerTy() ||
       SDVTList DeclareRetVTs = DAG.getVTList(MVT::Other, MVT::Glue);
       SDValue DeclareRetOps[] = { Chain, DAG.getConstant(1, dl, MVT::i32),
                                   DAG.getConstant(resultsz, dl, MVT::i32),
                                   DAG.getConstant(0, dl, MVT::i32), InFlag };
       Chain = DAG.getNode(NVPTXISD::DeclareRet, dl, DeclareRetVTs,
                           DeclareRetOps);
       InFlag = Chain.getValue(1);
     } else {
-      retAlignment = getArgumentAlignment(Callee, CS, retTy, 0);
+      retAlignment = getArgumentAlignment(Callee, CS, retTy, 0, DL);
       SDVTList DeclareRetVTs = DAG.getVTList(MVT::Other, MVT::Glue);
       SDValue DeclareRetOps[] = { Chain,
                                   DAG.getConstant(retAlignment, dl, MVT::i32),
                                   DAG.getConstant(resultsz / 8, dl, MVT::i32),
                                   DAG.getConstant(0, dl, MVT::i32), InFlag };
       Chain = DAG.getNode(NVPTXISD::DeclareRetParam, dl, DeclareRetVTs,
                           DeclareRetOps);
       InFlag = Chain.getValue(1);
if (retTy && retTy->isVectorTy()) {
             InVals.push_back(Elt);
           }
           Ofst += DL.getTypeAllocSize(VecVT.getTypeForEVT(F->getContext()));
         }
       }
     } else {
       SmallVector<EVT, 16> VTs;
       SmallVector<uint64_t, 16> Offsets;
-      ComputePTXValueVTs(*this, DAG.getDataLayout(), retTy, VTs, &Offsets, 0);
+      auto &DL = DAG.getDataLayout();
+      ComputePTXValueVTs(*this, DL, retTy, VTs, &Offsets, 0);
       assert(VTs.size() == Ins.size() && "Bad value decomposition");
-      unsigned RetAlign = getArgumentAlignment(Callee, CS, retTy, 0);
+      unsigned RetAlign = getArgumentAlignment(Callee, CS, retTy, 0, DL);
       for (unsigned i = 0, e = Ins.size(); i != e; ++i) {
         unsigned sz = VTs[i].getSizeInBits();
         unsigned AlignI = GreatestCommonDivisor64(RetAlign, Offsets[i]);
         bool needTruncate = false;
         if (VTs[i].isInteger() && sz < 8) {
           sz = 8;
           needTruncate = true;
         }

test/CodeGen/NVPTX/zero-cs.ll

+; RUN: not llc < %s -march=nvptx 2>&1 | FileCheck %s
+; used to seqfault and now fails with a "Cannot select"
+
+; CHECK: LLVM ERROR: Cannot select: t7: i32 = ExternalSymbol'__powidf2'
+define double @powi() {
+  %1 = call double @llvm.powi.f64(double 1.000000e+00, i32 undef)
+  ret double %1
+}
+
+declare double @llvm.powi.f64(double, i32) nounwind readnone