[PATCH] D142254: [X86] Transform vector SET{LE/ULT/ULE} -> SETLT and SET{GE/UGT/UGE} -> SETGT if possible

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D142254#4130525 <https://reviews.llvm.org/D142254#4130525>, @saugustine wrote:

> The fix does work. But I'm still trying to get a reproducer for the future.
>
> Incidentally, the function comment explicitly says "or this is not a simple vector constant", and yet originally didn't check for it actually being simple. With the updated fix it does.
>
> So I think this is correct.

Okay, I'll wait a bit to commit incase anyone else wants to weigh in.
Thanks for the report + fix + testing.

> In D142254#4130495 <https://reviews.llvm.org/D142254#4130495>, @goldstein.w.n wrote:
>
>> In D142254#4130325 <https://reviews.llvm.org/D142254#4130325>, @saugustine wrote:
>>
>>> I think it probably needs to also check if the type is Simple, because the exact assertion that fails is isSimple.
>>>
>>> And if I insert a check for isSimple on line 24731, the test does pass.
>>>
>>>   if (!BV || !V.getValueType().isSimple())
>>>
>>> Not sure if that is the correct fix, but perhaps better than reverting. The test case is huge and buried.
>>
>> Are you going to be able to confirm that fix works? Or is it too complicated to reproduce?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142254