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

Phoebe Wang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 9 00:48:57 PST 2022


pengfei added a comment.

In D123284#3982339 <https://reviews.llvm.org/D123284#3982339>, @jdoerfert wrote:

> 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.`

I explained above: `The case was drastically reduced from a big IR. It won't generate valid assembly but demonstrates the problem.`. I thought it addressed your concern :)
It's not clear to me why it triggers UB. All I can see is it won't generate real assemble due to other optimizations. But I think it is clear and legal to both argument promotion and inlining pass.

> 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.

Sure. I'm trying to address the comments.
I may not good at documentation. But I'd argue documentation is a relative subjective thing. And it's common a speaker thought he/she had explained clearly but audiences thought nonsense. That's why sometimes we need several rounds discussions/explanations in one topic.
So when I wrote "we can always improve documentation", I mean fine, let's just improve it. I don't find anything improper in the reply. Can you explain it more or suggest a better way how I should follow up?

> 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?

I think Craig helped addressed it.


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