PATCH: R600/SI: Experimental assembler / inline assembly support
Tom Stellard
tom at stellard.net
Fri Mar 20 14:41:58 PDT 2015
Hi Matt,
Here are updated patches that address the rest of your comments,
except for the one about the float cast, because I wasn't sure how to
fix it.
-Tom
On Fri, Mar 20, 2015 at 11:43:17AM -0400, Tom Stellard wrote:
> 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);
> > > +}
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-R600-SI-Don-t-print-offset0-offset1-DS-operands-when.patch
Type: text/x-diff
Size: 22889 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/b656a268/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-R600-SI-Add-missing-SOPK-instructions.patch
Type: text/x-diff
Size: 5865 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/b656a268/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-R600-SI-Initial-support-for-assembler-and-inline-ass.patch
Type: text/x-diff
Size: 154286 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/b656a268/attachment-0002.patch>
More information about the llvm-commits
mailing list