PATCH: R600/SI: Experimental assembler / inline assembly support
Tom Stellard
tom at stellard.net
Fri Mar 20 08:43:17 PDT 2015
Hi Matt,
I'm working on an update patch. Here are some responses to your
comments:
On Fri, Mar 13, 2015 at 10:47:38AM -0700, Matt Arsenault wrote:
> > + case Match_InvalidOperand: {
> > + SMLoc ErrorLoc = IDLoc;
> > + if (ErrorInfo != ~0ULL) {
> > + if (ErrorInfo >= Operands.size()) {
> > + return Error(IDLoc, "too few operands for instruction");
> > + }
> >
> > + ErrorLoc = ((AMDGPUOperand &)*Operands[ErrorInfo]).getStartLoc();
> Casting to a reference always looks weird, and there are a lot of these
> in this patch. Why do you need to do this? Can you not cast the pointer
> type before the deref and use -> for some weird reason?
Operands is a vector of std::unique_ptr, so you can't cast is as a
regular pointer type. Every other assembler casts as reference
like this.
> > @@ -195,17 +564,104 @@ AMDGPUAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
> >
> > // If we successfully parsed the operand or if there as an error parsing,
> > // we are done.
> > - if (ResTy == MatchOperand_Success || ResTy == MatchOperand_ParseFail)
> > + //
> > + // If we are parsing after we reach EndOfStatement then this means we
> > + // are appending default values to the Operands list. This is only done
> > + // by custom parser, so we shouldn't continue on to the generic parsing.
> > + if (ResTy == MatchOperand_Success || ResTy == MatchOperand_ParseFail ||
> > + getLexer().is(AsmToken::EndOfStatement))
> > return ResTy;
> >
> > + bool Negate = false, Abs = false;
> > + if (getLexer().getKind()== AsmToken::Minus) {
> > + Parser.Lex();
> > + Negate = true;
> > + }
> > +
> > + if (getLexer().getKind() == AsmToken::Pipe) {
> > + Parser.Lex();
> > + Abs = true;
> > + }
> > +
> > switch(getLexer().getKind()) {
> > case AsmToken::Integer: {
> > + SMLoc S = Parser.getTok().getLoc();
> > + int64_t IntVal;
> > + if (getParser().parseAbsoluteExpression(IntVal))
> > + return MatchOperand_ParseFail;
> > + APInt IntVal32(32, IntVal);
> > + if (IntVal32.getSExtValue() != IntVal) {
> > + Error(S, "invalid immediate: only 32-bit values are legal");
> > + return MatchOperand_ParseFail;
> > + }
> > +
> > + IntVal = IntVal32.getSExtValue();
> > + if (Negate)
> > + IntVal *= -1;
> > + Operands.push_back(AMDGPUOperand::CreateImm(IntVal, S));
> > + return MatchOperand_Success;
> > + }
> > + case AsmToken::Real: {
> > + // FIXME: We should emit an error if a double precisions floating-point
> > + // value is used. I'm not sure the best way to detect this.
> > + SMLoc S = Parser.getTok().getLoc();
> > int64_t IntVal;
> > if (getParser().parseAbsoluteExpression(IntVal))
> > return MatchOperand_ParseFail;
> > - Operands.push_back(AMDGPUOperand::CreateImm(IntVal));
> > +
> > + APFloat F((float)APInt(64, IntVal).bitsToDouble());
> You should be able to avoid using the host float cast here
What should I do instead?
> > + if (Negate)
> > + F.changeSign();
> > + Operands.push_back(
> > + AMDGPUOperand::CreateImm(F.bitcastToAPInt().getZExtValue(), S));
> > return MatchOperand_Success;
> > }
> > + case AsmToken::Identifier: {
> > + SMLoc S, E;
> > + unsigned RegNo;
> > + if (!ParseRegister(RegNo, S, E)) {
> > +
> > + bool HasModifiers = operandsHaveModifiers(Operands);
> > + unsigned Modifiers = 0;
> > +
> > + if (Negate)
> > + Modifiers |= 0x1;
> > +
> > + if (Abs) {
> > + if (getLexer().getKind() != AsmToken::Pipe)
> > + return MatchOperand_ParseFail;
> > + Parser.Lex();
> > + Modifiers |= 0x2;
> > + }
> > +
> > + if (Modifiers && !HasModifiers) {
> > + // We are adding a modifier to src1 or src2 and previous sources
> > + // don't have modifiers, so we need to go back and empty modifers
> > + // for each previous source.
> > + for (unsigned PrevRegIdx = Operands.size() - 1; PrevRegIdx > 1;
> > + --PrevRegIdx) {
> > +
> > + AMDGPUOperand &RegOp = ((AMDGPUOperand&)*Operands[PrevRegIdx]);
> > + RegOp.setModifiers(0);
> > + }
> > + }
> > +
> > +
> > + Operands.push_back(AMDGPUOperand::CreateReg(
> > + RegNo, S, E, getContext().getRegisterInfo()));
> > +
> > + if (HasModifiers || Modifiers) {
> > + AMDGPUOperand &RegOp = ((AMDGPUOperand&)*Operands[Operands.size() - 1]);
> > + RegOp.setModifiers(Modifiers);
> > +
> > + }
> > + } else {
> > + Operands.push_back(AMDGPUOperand::CreateToken(Parser.getTok().getString(),
> > + S));
> > + Parser.Lex();
> > + }
> > + return MatchOperand_Success;
> > + }
> > default:
> > return MatchOperand_NoMatch;
> > }
> > +void AMDGPUAsmParser::cvtMubuf(MCInst &Inst,
> > + const OperandVector &Operands) {
> > + unsigned i = 1;
> > +
> > + std::map<enum AMDGPUOperand::ImmTy, unsigned> OptionalIdx;
> > +
> > + for (unsigned e = Operands.size(); i != e; ++i) {
> This loop condition looks weird. You don't seem to be using the i after
> the loop, so its definition should move into the for
> > + AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
> > +
> > + // Add the register arguments
> > + if (Op.isReg()) {
> > + Op.addRegOperands(Inst, 1);
> > + continue;
> > + }
> > +
> > + // Handle the case where soffset is an immediate
> > + if (Op.isImm() && Op.getImmTy() == AMDGPUOperand::ImmTyNone) {
> > + Op.addImmOperands(Inst, 1);
> > + continue;
> > + }
> > +
> > + // Handle tokens like 'offen' which are sometimes hard-coded into the
> > + // asm string. There are no MCInst operands for these.
> > + if (Op.isToken()) {
> > + continue;
> > + }
> > + assert(Op.isImm());
> > +
> > + // Handle optional arguments
> > + OptionalIdx[Op.getImmTy()] = i;
> > + }
> > +
> > + assert(OptionalIdx.size() == 4);
> > +
> > + unsigned OffsetIdx = OptionalIdx[AMDGPUOperand::ImmTyOffset];
> > + unsigned GLCIdx = OptionalIdx[AMDGPUOperand::ImmTyGLC];
> > + unsigned SLCIdx = OptionalIdx[AMDGPUOperand::ImmTySLC];
> > + unsigned TFEIdx = OptionalIdx[AMDGPUOperand::ImmTyTFE];
> > +
> > + ((AMDGPUOperand &)*Operands[OffsetIdx]).addImmOperands(Inst, 1);
> > + ((AMDGPUOperand &)*Operands[GLCIdx]).addImmOperands(Inst, 1);
> > + ((AMDGPUOperand &)*Operands[SLCIdx]).addImmOperands(Inst, 1);
> > + ((AMDGPUOperand &)*Operands[TFEIdx]).addImmOperands(Inst, 1);
> Is this defaulting Offset/GLC/SLC/TFE to 1? Shouldn't these be 0?
No, 1 is the number of operands that should be added to the instruction.
> > +}
> > +
> > +//===----------------------------------------------------------------------===//
> > +// SI Inline Assembly Support
> > +//===----------------------------------------------------------------------===//
> > +
> > +std::pair<unsigned, const TargetRegisterClass *>
> > +SITargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
> > + const std::string &Constraint,
> StringRef operand instead?
It's a virtual function, so I'm don't think I can change it.
> > + MVT VT) const {
> > + dbgs() << "Constraint = " << Constraint << "\n";
> > + dbgs() << "VT = " << EVT(VT).getEVTString() << "\n";
> Leftover debug printing
> > + if (Constraint == "r") {
> > + switch(VT.SimpleTy) {
> > + default: llvm_unreachable("Unhandled type for 'r' inline asm constraint");
> > + case MVT::i64:
> > + return std::make_pair(0U, &AMDGPU::SGPR_64RegClass);
> > + case MVT::i32:
> > + return std::make_pair(0U, &AMDGPU::SGPR_32RegClass);
> > + }
> > + }
> > +
> > + if (Constraint.size() > 1) {
> > + const TargetRegisterClass *RC = nullptr;
> > + if (Constraint[1] == 'v') {
> > + RC = &AMDGPU::VGPR_32RegClass;
> > + } else if (Constraint[1] == 's') {
> > + RC = &AMDGPU::SGPR_32RegClass;
> > + }
> > +
> > + if (RC) {
> > + unsigned Idx = std::atoi(Constraint.substr(2).c_str());
> > + if (Idx < RC->getNumRegs())
> > + return std::make_pair(RC->getRegister(Idx), RC);
> > + }
> > + }
> > + return TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT);
> > +}
More information about the llvm-commits
mailing list