[PATCH] D122381: [NVPTX] Remove code duplication in LowerCall

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

Nice. LGTM with few style nits.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1476
+    if (IsByVal) {
+      // The ByValAlign in the Outs[OIdx].Flags is alway set at this point,
+      // so we don't need to worry about natural alignment or not.
----------------
alway->always


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1477
+      // The ByValAlign in the Outs[OIdx].Flags is alway set at this point,
+      // so we don't need to worry about natural alignment or not.
+      // See TargetLowering::LowerCallTo().
----------------
"whether it's naturally aligned or not."


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1479-1490
+      ArgAlign = Outs[OIdx].Flags.getNonZeroByValAlign();
+
+      // Try to increase alignment to enhance vectorization options.
+      const Function *F = CB->getCalledFunction();
+      Align AlignCandidate = getFunctionParamOptimizedAlign(F, ETy, DL);
+      ArgAlign = std::max(AlignCandidate, ArgAlign);
+
----------------
This all could be folded:
```
ArgAlign = std::max(
   std::max(Outs[OIdx].Flags.getNonZeroByValAlign(), 
                   getFunctionParamOptimizedAlign(CB->getCalledFunction(), ETy, DL)),
   4
);
```

It could be further extended to incorporate the `if (IsByVal)` -> `IsByVal ? ...`.

I think calculating ArgAlign in one place is a bit easier to follow than a series of conditional adjustments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122381/new/

https://reviews.llvm.org/D122381