Don't assert on a non-pointer value being used for a "p" inline asm constraint.

[aemerson]

Summary

GCC and existing codebases allow the use of integral values to be used
with this constraint. A recent change D133914 in this area started causing asserts.
Removing the assert is enough as the rest of the code works fine.

rdar://109675485

[pengfei]

Do you have a C code? I want to see its original constraint. I'm not sure the problem is in IR's constraint or the FE that chooses p as constraint. From LangRef , p is described as An address operand. I would incline to a restricted and precise semantic for constraints in IR. Otherwise, we may face more backend crash.

[aemerson]

As I mentioned there is *already* code out in the wild that are relying on this to work. GCC also accepts this code, and so IMO it is the defacto semantics of this feature. In this case the IR semantics are wrong.

As it currently stands clang with asserts will crash on code that it previously compiled cleanly.

[aemerson]

The be clear, the original c constraint was also p just like in the IR. I just used the output of -emit-llvm to write an IR test.

[arsenm]

It doesn't matter if it's invalid, the compiler shouldn't assert

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

9374

This is also assuming address space 0

[pengfei]

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

[aemerson]

In 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, @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).

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 that introduced this regression.

[pengfei]

In D155023#4496004, @aemerson wrote:

In 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, @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

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 that introduced this regression.

The story started from D122220 rather than D133914. If there is a clear declaration for p constraint, I'd suggest to improve D133914, e.g., changing it to i for immediate value.

[pengfei]

I found it works too if changing to ip there.

[aemerson]

In D155023#4496133, @pengfei wrote:

In D155023#4496004, @aemerson wrote:

In 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, @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 that introduced this regression.

The story started from D122220 rather than D133914. If there is a clear declaration for p constraint, I'd suggest to improve 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?

[pengfei]

In D155023#4496222, @aemerson wrote:

In D155023#4496133, @pengfei wrote:

In D155023#4496004, @aemerson wrote:

In 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, @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 that introduced this regression.

The story started from D122220 rather than D133914. If there is a clear declaration for p constraint, I'd suggest to improve 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 like this 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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case InlineAsm::isLabel: {
         AsmNodeOperands.push_back(DAG.getTargetConstant(ResOpType,
                                                         getCurSDLoc(),
                                                         MVT::i32));
         AsmNodeOperands.push_back(InOperandVal);
         break;
       }
 
       if (OpInfo.ConstraintType == TargetLowering::C_Address) {
-        assert(InOperandVal.getValueType() ==
-                   TLI.getPointerTy(DAG.getDataLayout()) &&
-               "Address operands expect pointer values");
-
         unsigned ConstraintID =
             TLI.getInlineAsmMemConstraint(OpInfo.ConstraintCode);
         assert(ConstraintID != InlineAsm::Constraint_Unknown &&
                "Failed to convert memory constraint code to constraint id.");
 
         unsigned ResOpType = InlineAsm::getFlagWord(InlineAsm::Kind_Mem, 1);
 
         SDValue AsmOp = InOperandVal;

llvm/test/CodeGen/X86/inline-asm-p-constraint.ll

; CHECK: movq    %rdi, -8(%rsp)
   %1 = tail call ptr asm "mov $1, $0\0A\09lea $2, $0", "=r,p,*m,~{dirflag},~{fpsr},~{flags}"(ptr %Ptr, ptr elementtype(ptr) %Ptr.addr)
 ; CHECK-NEXT: #APP
 ; CHECK-NEXT: mov (%rdi), %rax
 ; CHECK-NEXT: lea -8(%rsp), %rax
 ; CHECK-NEXT: #NO_APP
   ret ptr %1
 ; CHECK-NEXT: retq
 }
+
+define void @intptr() {
+; Don't assert on a non-ptr operand, existing code & gcc accept these.
+entry:
+; CHECK-LABEL: intptr:
+; CHECK: ud1l 49150(%eax), %eax
+  call void asm "ud1l $0(%eax), %eax", "p,~{dirflag},~{fpsr},~{flags}"(i32 49150)
+  unreachable
+}