Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Justin Lebar via cfe-commits]

jlebar added a comment.

> 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.


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Justin Lebar via cfe-commits]

jlebar added a subscriber: jlebar.
jlebar added a comment.

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


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy added a comment.

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


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy updated this revision to Diff 71951.
vchuravy added a comment.

Fix spelling in comment


https://reviews.llvm.org/D9168

Files:
  lib/Target/NVPTX/NVPTXISelLowering.cpp
  lib/Target/NVPTX/NVPTXISelLowering.h
  test/CodeGen/NVPTX/zero-cs.ll

Index: test/CodeGen/NVPTX/zero-cs.ll
===
--- /dev/null
+++ test/CodeGen/NVPTX/zero-cs.ll
@@ -0,0 +1,10 @@
+; 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.00e+00, i32 undef)
+  ret double %1
+}
+
+declare double @llvm.powi.f64(double, i32) nounwind readnone
Index: lib/Target/NVPTX/NVPTXISelLowering.h
===
--- lib/Target/NVPTX/NVPTXISelLowering.h
+++ lib/Target/NVPTX/NVPTXISelLowering.h
@@ -539,7 +539,8 @@
   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
 
Index: lib/Target/NVPTX/NVPTXISelLowering.cpp
===
--- lib/Target/NVPTX/NVPTXISelLowering.cpp
+++ lib/Target/NVPTX/NVPTXISelLowering.cpp
@@ -1024,11 +1024,15 @@
   return O.str();
 }
 
-unsigned
-NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
-  const ImmutableCallSite *CS,
-  Type *Ty,
-  unsigned Idx) const {
+unsigned NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
+   const ImmutableCallSite *CS,
+   Type *Ty, unsigned Idx,
+   const DataLayout &DL) const {
+  if (!CS) {
+// CallSite is zero, fallback to ABI type alignment
+return DL.getABITypeAlignment(Ty);
+  }
+
   unsigned Align = 0;
   const Value *DirectCallee = CS->getCalledFunction();
 
@@ -1046,7 +1050,7 @@
 
   const Value *CalleeV = cast(CalleeI)->getCalledValue();
   // Ignore any bitcast instructions
-  while(isa(CalleeV)) {
+  while (isa(CalleeV)) {
 const ConstantExpr *CE = cast(CalleeV);
 if (!CE->isCast())
   break;
@@ -1069,7 +1073,6 @@
 
   // 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);
 }
 
@@ -1126,7 +1129,8 @@
 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  .b8 .param[];
 unsigned sz = DL.getTypeAllocSize(Ty);
 SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
@@ -1166,7 +1170,8 @@
   }
   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  .b8 .param[];
 unsigned sz = DL.getTypeAllocSize(Ty);
 SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
@@ -1426,7 +1431,7 @@
   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),
@@ -1633,9 +1638,10 @@
 } else {
   SmallVector VTs;
   SmallVector 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]);
___
cfe-co

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Jacques Pienaar via cfe-commits]

jpienaar accepted this revision.
jpienaar added a reviewer: jpienaar.
jpienaar added a comment.
This revision is now accepted and ready to land.

Looks good to me, do you need help submitting?



Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1032
@@ +1031,3 @@
+  if (!CS) {
+// callsize is zero, fallback to ABI type alignment
+return DL.getABITypeAlignment(Ty);

s/callsize/Call site/ ?


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy updated this revision to Diff 71946.
vchuravy added a comment.

addresses review comments.


https://reviews.llvm.org/D9168

Files:
  lib/Target/NVPTX/NVPTXISelLowering.cpp
  lib/Target/NVPTX/NVPTXISelLowering.h
  test/CodeGen/NVPTX/zero-cs.ll

Index: test/CodeGen/NVPTX/zero-cs.ll
===
--- /dev/null
+++ test/CodeGen/NVPTX/zero-cs.ll
@@ -0,0 +1,10 @@
+; 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.00e+00, i32 undef)
+  ret double %1
+}
+
+declare double @llvm.powi.f64(double, i32) nounwind readnone
Index: lib/Target/NVPTX/NVPTXISelLowering.h
===
--- lib/Target/NVPTX/NVPTXISelLowering.h
+++ lib/Target/NVPTX/NVPTXISelLowering.h
@@ -539,7 +539,8 @@
   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
 
Index: lib/Target/NVPTX/NVPTXISelLowering.cpp
===
--- lib/Target/NVPTX/NVPTXISelLowering.cpp
+++ lib/Target/NVPTX/NVPTXISelLowering.cpp
@@ -1024,11 +1024,15 @@
   return O.str();
 }
 
-unsigned
-NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
-  const ImmutableCallSite *CS,
-  Type *Ty,
-  unsigned Idx) const {
+unsigned NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
+   const ImmutableCallSite *CS,
+   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();
 
@@ -1046,7 +1050,7 @@
 
   const Value *CalleeV = cast(CalleeI)->getCalledValue();
   // Ignore any bitcast instructions
-  while(isa(CalleeV)) {
+  while (isa(CalleeV)) {
 const ConstantExpr *CE = cast(CalleeV);
 if (!CE->isCast())
   break;
@@ -1069,7 +1073,6 @@
 
   // 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);
 }
 
@@ -1126,7 +1129,8 @@
 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  .b8 .param[];
 unsigned sz = DL.getTypeAllocSize(Ty);
 SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
@@ -1166,7 +1170,8 @@
   }
   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  .b8 .param[];
 unsigned sz = DL.getTypeAllocSize(Ty);
 SDVTList DeclareParamVTs = DAG.getVTList(MVT::Other, MVT::Glue);
@@ -1426,7 +1431,7 @@
   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),
@@ -1633,9 +1638,10 @@
 } else {
   SmallVector VTs;
   SmallVector 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]);
___
cfe

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy added a comment.

>   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.


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Jacques Pienaar via cfe-commits]

jpienaar added a comment.

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.



Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1033
@@ +1032,3 @@
+  const DataLayout &DL) const {
+  if (CS) {
+unsigned Align = 0;

There is a preference to early exits. So perhaps:
  if (!CS)
return DL.getABITTypeAlignment(Ty);


Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1131
@@ -1128,3 +1130,3 @@
 
-unsigned align = getArgumentAlignment(Callee, CS, Ty, paramCount + 1);
+unsigned align = getArgumentAlignment(Callee, CS, Ty, paramCount + 1, 
DL);
 // declare .param .align  .b8 .param[];

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 


Comment at: test/CodeGen/NVPTX/zero-cs.ll:1
@@ +1,2 @@
+; RUN: not llc < %s -march=nvptx 2>&1 | FileCheck %s
+

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)?


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy updated this revision to Diff 71936.
vchuravy added a comment.

Rebases the original changes and adds a test-case


https://reviews.llvm.org/D9168

Files:
  lib/Target/NVPTX/NVPTXISelLowering.cpp
  lib/Target/NVPTX/NVPTXISelLowering.h
  test/CodeGen/NVPTX/zero-cs.ll

Index: test/CodeGen/NVPTX/zero-cs.ll
===
--- /dev/null
+++ test/CodeGen/NVPTX/zero-cs.ll
@@ -0,0 +1,9 @@
+; RUN: not llc < %s -march=nvptx 2>&1 | FileCheck %s
+
+; CHECK: LLVM ERROR: Cannot select: t7: i32 = ExternalSymbol'__powidf2'
+define double @powi() {
+  %1 = call double @llvm.powi.f64(double 1.00e+00, i32 undef)
+  ret double %1
+}
+
+declare double @llvm.powi.f64(double, i32) nounwind readnone
Index: lib/Target/NVPTX/NVPTXISelLowering.h
===
--- lib/Target/NVPTX/NVPTXISelLowering.h
+++ lib/Target/NVPTX/NVPTXISelLowering.h
@@ -539,7 +539,7 @@
   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
 
Index: lib/Target/NVPTX/NVPTXISelLowering.cpp
===
--- lib/Target/NVPTX/NVPTXISelLowering.cpp
+++ lib/Target/NVPTX/NVPTXISelLowering.cpp
@@ -1028,48 +1028,50 @@
 NVPTXTargetLowering::getArgumentAlignment(SDValue Callee,
   const ImmutableCallSite *CS,
   Type *Ty,
-  unsigned Idx) const {
-  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(CalleeI)) {
-  // Check if we have call alignment metadata
-  if (llvm::getAlign(*cast(CalleeI), Idx, Align))
-return Align;
+  unsigned Idx,
+  const DataLayout &DL) const {
+  if (CS) {
+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(CalleeI)) {
+// Check if we have call alignment metadata
+if (llvm::getAlign(*cast(CalleeI), Idx, Align))
+  return Align;
+
+const Value *CalleeV = cast(CalleeI)->getCalledValue();
+// Ignore any bitcast instructions
+while(isa(CalleeV)) {
+  const ConstantExpr *CE = cast(CalleeV);
+  if (!CE->isCast())
+break;
+  // Look through the bitcast
+  CalleeV = cast(CalleeV)->getOperand(0);
+}
 
-  const Value *CalleeV = cast(CalleeI)->getCalledValue();
-  // Ignore any bitcast instructions
-  while(isa(CalleeV)) {
-const ConstantExpr *CE = cast(CalleeV);
-if (!CE->isCast())
-  break;
-// Look through the bitcast
-CalleeV = cast(CalleeV)->getOperand(0);
+// We have now looked past all of the bitcasts.  Do we finally have a
+// Function?
+if (isa(CalleeV))
+  DirectCallee = CalleeV;
   }
-
-  // We have now looked past all of the bitcasts.  Do we finally have a
-  // Function?
-  if (isa(CalleeV))
-DirectCallee = CalleeV;
 }
-  }
 
-  // Check for function alignment information if we found that the
-  // ultimate target is a Function
-  if (DirectCallee)
-if (llvm::getAlign(*cast(DirectCallee), Idx, Align))
-  return Align;
+// Check for function alignment information if we found that the
+// ultimate target is a Function
+if (DirectCallee)
+  if (llvm::getAlign(*cast(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);
 }
 
@@ -1126,7 +1128,7 @@
 ComputePTXValueVTs(*this, DAG.getDataLayout(), Ty, vtparts, &Offsets,
0);
 
-unsigned align = getArgumentAlignment(Callee, CS, Ty, paramCount + 1);
+unsigned align = getArgu

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy added a comment.

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


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Jacques Pienaar via cfe-commits]

@Valentin: It would be great if you could give it a go. I tried
resurrecting it earlier today but noticed getDataLayout() had been removed
and then I got tied up with other pending work.

On Mon, Sep 19, 2016 at 5:11 PM, Valentin Churavy 
wrote:

> vchuravy added a comment.
>
> @jpienaar are you planning to work on this again? Or should I give it a go?
>
>
> https://reviews.llvm.org/D9168
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy added a comment.

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


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Valentin Churavy via cfe-commits]

vchuravy added a comment.

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.

F2433038: strip.ll 


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D9168: [NVPTX] Check if callsite is defined when computing argument allignment

[Jacques Pienaar via cfe-commits]

jpienaar added a comment.

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


https://reviews.llvm.org/D9168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits