[PATCH] D34708: [NVPTX] Allow to make libcalls that are defined in the current module.

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1314-1315
+            align = DL.getABITypeAlignment(Ty);
+        }
+        else {
+           align = DL.getABITypeAlignment(Ty);
----------------
Nit. these should be on the same line.


================
Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1631-1632
 
+  // Libcalls doesn't have call site and but they still are NOT indirect calls.  
+  bool isIndirectCall = !Func && CS;
+
----------------
I'd move it down to where isIndirectCall is used.

Also, I'd rephrase the comment along the lines of 

```
Both indirect calls and libcalls have nullptr Func. In order to distinguish between them we must rely on the call site value which is valid for indirect calls but is always null for libcalls.
```

I wonder if there's any way to explicitly mark the call as libcall, so we don't have to rely on callsite for that.


================
Comment at: lib/Target/NVPTX/NVPTXISelLowering.cpp:1841-1848
+  if (!CS) {
+    // Unfortunately, libcall expansion does not respect `Chain`...
+    // Our `CALLSEQ_END` becomes dead node (i.e. nobody references it) because
+    // only retval `LoadParam`s will be used after expansion. That means the
+    // node will be deleted at the next iteration of legalization and we have
+    // to make effors to keep it alive.
+    DAG.KeepNodeAlive(Chain.getNode(), true);
----------------
denzp wrote:
> arsenm wrote:
> > This is probably the wrong way to do this. You probably want to do a getMergeValues with the returned value and the entry chain
> Thanks, it's a good idea and a right way. But... we are still losing reference during legalization:
> 
> ```
> Legalizing: t28: f64,ch = merge_values t25, t27
> t28: f64,ch = merge_values t25, t27 ... replacing: t28: f64,ch = merge_values t25, t27
>      with:      t25: f64,ch,glue = NVPTXISD::LoadParam<LD8[<unknown>]> t24, Constant:i32<1>, Constant:i32<0>, t24:1
>       and:      t27: ch,glue = callseq_end t25:1, TargetConstant:i32<0>, TargetConstant:i32<1>, t25:2
> ```
> 
> Unfortunately, nobody is referencing `t27` and it's being removed as a dead node.
I guess we could modify  NVPTXISD::StoreRetval so it takes additional 'glue' argument and explicitly make it depend on glue returned by callseq_end. Perhaps in a separate patch.


https://reviews.llvm.org/D34708