[PATCH] D155023: Don't assert on a non-pointer value being used for a "p" inline asm constraint.

[Phoebe Wang via Phabricator via llvm-commits]

pengfei accepted this revision.
pengfei added a comment.
This revision is now accepted and ready to land.

In D155023#4496222 <https://reviews.llvm.org/D155023#4496222>, @aemerson wrote:

> In D155023#4496133 <https://reviews.llvm.org/D155023#4496133>, @pengfei wrote:
>
>> In D155023#4496004 <https://reviews.llvm.org/D155023#4496004>, @aemerson wrote:
>>
>>> In D155023#4495929 <https://reviews.llvm.org/D155023#4495929>, @pengfei wrote:
>>>
>>>> My concern is removing assert don't solve the real problem. I think there are two problems here:
>>>>
>>>> 1. Compiler shouldn't assert for invalid semantics.  That's true. We could solve it by detecting such invalid semantics during sema checking.
>>>> 2. Application relies on invalid semantics. This is my main concern. Besides, I took a look at GCC and find it report error for it https://godbolt.org/z/zPWhsY5aj
>>>
>>>
>>>
>>> In D155023#4495929 <https://reviews.llvm.org/D155023#4495929>, @pengfei wrote:
>>>
>>>> My concern is removing assert don't solve the real problem. I think there are two problems here:
>>>>
>>>> 1. Compiler shouldn't assert for invalid semantics.  That's true. We could solve it by detecting such invalid semantics during sema checking.
>>>> 2. Application relies on invalid semantics. This is my main concern. Besides, I took a look at GCC and find it report error for it https://godbolt.org/z/zPWhsY5aj
>>>
>>> Sorry the inline assembly in the test isn't actually valid for ud1l, here's an example showing 'p' being used with an integer literal: https://godbolt.org/z/sPqqj5q8M
>>>
>>>   void *test()
>>>   {
>>>     void *ret;
>>>     asm ("movq %1, %0" : "=r" (ret) : "p" (256));
>>>     return ret;
>>>   }
>>>
>>> You'll find that gcc happily accepts this. For non-standardized features like this, the semantics are defined by what the implementations *do*. If GCC has always rejected this as an error, I'm happy to do the same. But GCC doesn't, so we should match them instead of breaking existing users' code (that includes throwing an error in Sema).
>>
>> That isn't convincing much since GCC doesn't document the `p` constraint somewhere. https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Machine-Constraints.html
>
> https://gcc.gnu.org/onlinedocs/gcc/Simple-Constraints.html ?
>
>>> Now if you are convinced that this deletion of the assert is incorrect, then we can discuss a better fix. The other option is that we entirely back out the original change D133914 <https://reviews.llvm.org/D133914> that introduced this regression.
>>
>> The story started from D122220 <https://reviews.llvm.org/D122220> rather than D133914 <https://reviews.llvm.org/D133914>. If there is a clear declaration for `p` constraint, I'd suggest to improve D133914 <https://reviews.llvm.org/D133914>, e.g., changing it to `i` for immediate value.
>
> I'm not exactly sure what you're suggesting. Ultimately: are we going to diverge from the canonical gcc behavior here for this feature?

Thanks for the link. Sorry, I suggested to improve D122220 <https://reviews.llvm.org/D122220> like this D155160 <https://reviews.llvm.org/D155160>

IR constraint doesn't necessarily equal to the C constraint, but we need to support the full semantics of a C constraint. Since GCC has document it well, I think it's fine to go with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155023