PowerPC: 'Z' inline assembly constraint not supported

[tstellar]

Bugzilla Link	50608
Resolution	FIXED
Resolved on	Jun 15, 2021 15:33
Version	trunk
OS	Linux
Blocks	#48661
CC	@nemanjai,@tbaederr
Fixed by commit(s)	a9e4f91 f78f530

Extended Description

cat test.c

void func(int myarg)
{
asm volatile(" .asciz "%n[S1]@%I[A1]%[A1]" " :: [S1] "n" (1), [A1] "nZr" ((myarg)));
}

int main(int argc, char **argv)
{
func(0);
return 0;
}

./bin/clang --target=ppc64le test.c

test.c:10:18: error: invalid operand in inline asm: ' .asciz "${0:n}@${1:I}$1" '

If I remove the 'Z constraint the test case compiles successfully. This constraint is supported by gcc: https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html

[tstellar]

systemtap uses this constraint, so some probes fail to compile when using clang:

https://bugzilla.redhat.com/show_bug.cgi?id=1967482

[tbaederr]

It also compiles when changing the inline asm from %I to %U:

asm volatile(" .asciz "%n[S1]@%U[A1]%[A1]" "
:: [S1] "n" (1), [A1] "nZr" ((myarg)));

This way the output from gcc -S is:

 .asciz "-1@0(9)"

and clang's output is:

.ascii	"-1@0(3)"

The same output can be produced by adding a case for 'I' to
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L362

[nemanjai]

I agree that adding a case for 'I' in the handling for memory operands appears to be the correct course of action here. Regardless of whether the memory operand is X-Form, D-Form or an update form, clang will load the value into a register so it seems to make sense they should be handled the same way.

[tbaederr]

I can provide a patch doing that but I don't know enough about power asm to add a comment to the case statement like the X and U have.

[tbaederr]

Nemanja,

https://llvm.org/docs/LangRef.html#asm-template-argument-modifiers documents 'I' as:

I: Print the letter ‘i’ if the operand is an integer constant, otherwise nothing. Used to print ‘addi’ vs ‘add’ instructions.

is handling it like X and U , which "LLVM does not support [...], so this will currently always print nothing" really correct? If that is right, I can provide a patch and a test case.

[nemanjai]

The printing of non-memory and memory ASM operands appears to be completely disjoint so I don't think this would present a problem. We would then probably need to improve the documentation to something like:

I: Print the letter ‘i’ if the operand is an integer constant, otherwise nothing. Used to print ‘addi’ vs ‘add’ instructions. (Note: when applied to a memory operand, it is treated similarly to ‘U‘ and ‘X‘)

I am just going to double-check with the GCC developers that this is inline with what GCC does.

[tbaederr]

Thanks. I've opened https://reviews.llvm.org/D103968 with the current changes in the meantime, so we can continue the review there.

[nemanjai]

Actually, I just checked. GCC does not handle this specially in any way for memory operands - it is still just an 'i' if the operand is an immediate. So the fix should just be to duplicate the case from PrintAsmOperand such as:

diff --git a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
index e6326ac4c2e3..613ed9c24b4a 100644
--- a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -359,6 +359,12 @@ bool PPCAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
O << "0, ";
printOperand(MI, OpNo, O);
return false;

• case 'I':

•  // Write 'i' if an integer constant, otherwise nothing.  Used to print
  

•  // addi vs add, etc.
  

•  if (MI->getOperand(OpNo).isImm())
  

•    O << "i";
  

•  return false;
  

  case 'U': // Print 'u' for update form.
  case 'X': // Print 'x' for indexed form.
  // FIXME: Currently for PowerPC memory operands are always loaded

And we do not need to add anything specific to the documentation. Thank you for looking into this and for your diligence.

[tbaederr]

I've pushed a9e4f91

[tstellar]

Nemanja, any objections to backporting this to the release/12.x branch?

[nemanjai]

Nemanja, any objections to backporting this to the release/12.x branch?

No objections or concerns, please go ahead.

[tstellar]

Merged: f78f530