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

[Daniil Kovalev via Phabricator via llvm-commits]

kovdan01 added inline comments.


================
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);
+
----------------
tra wrote:
> 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.
Creating a one-liner here does not look very good as for me - after clang-format we get the following code:

```
Align ArgAlign =
        IsByVal
            ? std::max(std::max(
                           // The ByValAlign in the Outs[OIdx].Flags is always
                           // set at this point, so we don't need to worry
                           // whether it's naturally aligned or not. See
                           // TargetLowering::LowerCallTo().
                           Outs[OIdx].Flags.getNonZeroByValAlign(),
                           // Try to increase alignment to enhance vectorization
                           // options.
                           getFunctionParamOptimizedAlign(
                               CB->getCalledFunction(), ETy, DL)),
                       // Enforce minumum alignment of 4 to work around ptxas
                       // miscompile for sm_50+. See corresponding alignment
                       // adjustment in emitFunctionParamList() for details.
                       Align(4))
            : getArgumentAlignment(Callee, CB, Ty, ParamCount + 1, DL);
```

I just removed some intermediate variables and used temporary values instead. Looks clear enough IMHO, and there is no problem in reading code like

```
// ...
ArgAlign = ...;
// ...
ArgAlign = std::max(ArgAlign, ...);
// ...
ArgAlign = std::max(ArgAlign, ...);
```


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

https://reviews.llvm.org/D122381