Page MenuHomePhabricator

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

Authored by jonpa on Nov 20 2019, 10:58 AM.

Details

Reviewers
uweigand
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
                     ^

Diff Detail

Event Timeline

jonpa created this revision.Nov 20 2019, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 10:58 AM

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 added a comment.Nov 21 2019, 1:15 AM

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?

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

jonpa updated this revision to Diff 230872.Nov 25 2019, 4:03 AM

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.

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 added a comment.Nov 25 2019, 4:30 AM

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.

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 updated this revision to Diff 231023.Nov 26 2019, 1:47 AM

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 accepted this revision.Nov 26 2019, 2:05 AM

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.

This revision is now accepted and ready to land.Nov 26 2019, 2:05 AM