[SystemZ] Don't build a PPA instruction with an immediate 0

[jonpa]

Summary

The improvement in the machine verifier for operand types (D63973) discovered a bad operand in a test using a PPA instruction. It was an immediate 0 where a register was expected.

This patch is an attempt to remedy this by using the same register in the second operand as in the first and then emitting instead %R0D in the assembly printer. I first tried to do away with the second register operand and just printing 0, but then some MC tests failed, so it seems we do need to model the second operand as a register to have all parts working.

This patch in fact improves things a bit, since currently it doesn't work to do

llc ~/llvm-project/llvm/test/CodeGen/SystemZ/htm-intrinsics.ll -mtriple=s390x-linux-gnu -mcpu=zEC12 -o out.s
llvm-mc -mcpu=zEC12 out.s --show-encoding
out.s:403:11: error: invalid operand for instruction
        ppa     %r2, 0, 1
                     ^

[uweigand]

I thought the 0 here was supposed to the the "magic" NoRegister register number. This is what is supposed to be used in a register slot if there's no register actually present, e.g. if an instruction has base+index address but we're not using an index in this instance.

Can you check why this isn't working as I thought?

[jonpa]

In D70501#1753929, @uweigand wrote:

I thought the 0 here was supposed to the the "magic" NoRegister register number. This is what is supposed to be used in a register slot if there's no register actually present, e.g. if an instruction has base+index address but we're not using an index in this instance.

Can you check why this isn't working as I thought?

It seems to me that AddressingMode operands have explicit casts to RegisterOperand, but I can't find a way to express this for a Pat. I tried to make it work with COPY_TO_REGCLASS and COPY but failed...

It seems logical that TableGen should infer that the 0 should mean register 0 since the PPA instruction has been declared that operand to be a register, but it instead builds an immediate. Not sure if this is something that could be fixed...

Maybe using a Pseudo instruction during isel and build the PPA in Finalize Isel would be an option?

[uweigand]

I see there's a special node "zero_reg" -- does this work?

[jonpa]

I see there's a special node "zero_reg" -- does this work?

Ah yes, that must be what I was looking for :-)

This builds a register operand with NoReg, which then makes the IR correct while it is also necessary to handle this in the AsmPrinter since otherwise the call to get the register name will fail.

[uweigand]

In D70501#1758612, @jonpa wrote:

This builds a register operand with NoReg, which then makes the IR correct while it is also necessary to handle this in the AsmPrinter since otherwise the call to get the register name will fail.

Can't we handle NoRegister generically in the AsmPrinter somewhere without tying that to the specific PPA opcode? Maybe we'll want to use it elsewhere as well ...

[jonpa]

In D70501#1758646, @uweigand wrote:

In D70501#1758612, @jonpa wrote:

This builds a register operand with NoReg, which then makes the IR correct while it is also necessary to handle this in the AsmPrinter since otherwise the call to get the register name will fail.

Can't we handle NoRegister generically in the AsmPrinter somewhere without tying that to the specific PPA opcode? Maybe we'll want to use it elsewhere as well ...

I thought that since generally a NoRegister could be the result of a bug somewhere in the compiler, it would be best to handle it as a special case only where it is expected. If that's not useful, I think it could be handled generally in SystemZInstPrinter.cpp, to just print '0' for any such operand. Seems like it would have to be handled both in SystemZInstPrinter::printOperand() and SystemZInstPrinter::printRegName(), but I'm not sure.

[uweigand]

In D70501#1758659, @jonpa wrote:

I thought that since generally a NoRegister could be the result of a bug somewhere in the compiler, it would be best to handle it as a special case only where it is expected. If that's not useful, I think it could be handled generally in SystemZInstPrinter.cpp, to just print '0' for any such operand. Seems like it would have to be handled both in SystemZInstPrinter::printOperand() and SystemZInstPrinter::printRegName(), but I'm not sure.

Right now I don't think this crash is a reliable check anyway -- e.g. if you directly emit machine code (not assembler text), you won't get a crash either.

So I think it should be OK to just have printOperand() accept NoRegister and print "0". It looks like printRegName() does not need to handle NoRegister; this is used by common code to print register names for CFI directives and the like, where we should never have NoRegister.

(Another interesting question is whether our AsmParser will actually correctly read back and accept "0" in a register slot ... we should probably test this as well.)

[jonpa]

SystemZInstPrinter::printOperand() prints '0' for NoRegister generally instead of the PPA special handling.

This is NFC, with the only difference that the PPA second register operand now is NoRegister instead of immediate 0 in the MIR.

Right now I don't think this crash is a reliable check anyway -- e.g. if you directly emit machine code (not assembler text), you won't get a crash either.

OK, that makes sense...

(Another interesting question is whether our AsmParser will actually correctly read back and accept "0" in a register slot ... we should probably test this as well.)

No, it can't:

llvm-mc -mcpu=zEC12 out.s --show-encoding
out.s:403:11: error: invalid operand for instruction
        ppa     %r2, 0, 1
                             ^

SystemZAsmParser::parseGR64() does not handle raw register numbers, it seems.

[uweigand]

In D70501#1759890, @jonpa wrote:

SystemZAsmParser::parseGR64() does not handle raw register numbers, it seems.

Hmm, it probably should, for gas compatibility if nothing else ... But that can be a separate issue.

This patch LGTM.

[jonpa]

3ec193f

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

case SystemZ::CondTrap: {
     const MCConstantExpr *ConstExpr = MCConstantExpr::create(2, OutContext);
     LoweredMI = MCInstBuilder(SystemZ::BRC)
       .addImm(MI->getOperand(0).getImm())
       .addImm(MI->getOperand(1).getImm())
       .addExpr(MCBinaryExpr::createAdd(Expr, ConstExpr, OutContext));
     }
     break;
 
+  case SystemZ::PPA:
+    assert(MI->getOperand(1).getReg() == SystemZ::NoRegister &&
+           "Expected PPA to have NoRegister as second register.");
+    LoweredMI = MCInstBuilder(SystemZ::PPA)
+      .addReg(MI->getOperand(0).getReg())
+      .addImm(0)
+      .addImm(MI->getOperand(2).getImm());
+    break;
+
   case TargetOpcode::FENTRY_CALL:
     LowerFENTRY_CALL(*MI, Lower);
     return;
 
   case TargetOpcode::STACKMAP:
     LowerSTACKMAP(*MI);
     return;
 

llvm/lib/Target/SystemZ/SystemZInstrInfo.td

 // Processor assist
 //===----------------------------------------------------------------------===//
 
 let Predicates = [FeatureProcessorAssist] in {
   let hasSideEffects = 1 in
     def PPA : SideEffectTernaryRRFc<"ppa", 0xB2E8, GR64, GR64, imm32zx4>;
   def : Pat<(int_s390_ppa_txassist GR32:$src),
             (PPA (INSERT_SUBREG (i64 (IMPLICIT_DEF)), GR32:$src, subreg_l32),
-                 0, 1)>;
+                 zero_reg, 1)>;
 }
 
 //===----------------------------------------------------------------------===//
 // Miscellaneous Instructions.
 //===----------------------------------------------------------------------===//
 
 // Find leftmost one, AKA count leading zeros.  The instruction actually
 // returns a pair of GR64s, the first giving the number of leading zeros