[PATCH] D141830: [NFC] Use new version of SelectionDAG::getLoad in NVPTX LowerFormalArguments

Guillaume Chatelet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 20 04:01:26 PST 2023


gchatelet added a comment.

In D141830#4067189 <https://reviews.llvm.org/D141830#4067189>, @tra wrote:

> The last revision of the patch just explicitly does what the DAG.getLoad() oveload we've picked before did under the hood. We've intentionally provided an overload which acceps unsigned value for alignment. I do not see much of a point Creating `Align()` manually. For what it's worth your first patch variant was more readable.
> Your diff still makes no functional changes.

This patch is indeed NFC (as the updated title suggests). There is no problem to fix apart reducing the number of places where we use integer types to convey alignment (or lack thereof).
The `Align` and `MaybeAlign`types convey the intent better (I originally thought that the `bool` promotion was a mistake). `Align` also guarantees that the alignment is a valid power of two and represents it tersely (one byte).
You can see the original RFC <https://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html> to see why this can be beneficial.

If you feel strongly about it I can abandon this patch but IMHO being more explicit doesn't hurt and tremendously helps with subsequent refactoring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141830



More information about the llvm-commits mailing list