[PATCH] D123284: [ArgPromotion][Attributor] Update min-legal-vector-width when do promotion

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 8 11:39:29 PST 2022


jdoerfert added a comment.

In D123284#3980815 <https://reviews.llvm.org/D123284#3980815>, @pengfei wrote:

> In D123284#3979541 <https://reviews.llvm.org/D123284#3979541>, @arsenm wrote:
>
>> This should not have landed without additional review. I have raised some issues at
>>
>> https://reviews.llvm.org/rG7c04454227f5c6bd3f515783950a815970c90558
>
> Explained there. This patch is simply a bug fix. The document just describes an existing design. No new implementation and change here.

To be fair, this went up for review but did never get approval. Committing it this way is discouraged. Further, comments made before have not been addressed, e.g.,
`4. The functions in the test could not have been executed without triggering UB. Hence, it would have been valid to remove the call and place an unreachable there instead. Valid, but obviously not what you wanted to test.`

All that said, there is post-commit review. Now @arsenm brings up valid points, esp. for the documentation which was lacking before (as per @nikic) and is still not useful.
Just arguing "it is somewhere there" or "we can always improve documentation" is not what I would encourage as response given the way this was landed.

Now to some technical issue:
Why would we even track this in an argument? We are apparently free to modify the IR no matter what the current argument says, but if we do we need to update it "for the (X86) backend". Why doesn't the (X86) backend simply check the argument types itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123284



More information about the llvm-commits mailing list