[PATCH] D122381: [NVPTX] Remove code duplication in LowerCall
Daniil Kovalev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 02:34:57 PDT 2022
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
More information about the llvm-commits
mailing list