Page MenuHomePhabricator

[AMDGPU] Assembler: Simplify handling of optional operands
ClosedPublic

Authored by nhaustov on Feb 19 2016, 6:29 AM.

Details

Summary

Prepare to support DPP encodings.

For DPP encodings, we want row_mask/bank_mask/bound_ctrl to be optional operands. However this means that when parsing instruction which has no mnemonic prefix, we cannot add both default values for VOP3 and for DPP optional operands to OperandVector - neither instructions would match. So add default values for optional operands to MCInst during conversion instead.

Mark more operands as IsOptional = 1 in .td files.
Do not add default values for optional operands to OperandVector in AMDGPUAsmParser.
Add default values for optional operands during conversion using new helper addOptionalImmOperand.
Change to cvtVOP3_2_mod to check instruction flag instead of presence of modifiers. In the future, cvtVOP3* functions can be combined into one.
Separate cvtFlat and cvtFlatAtomic.
Fix CNDMASK_B32 definition to have no modifiers.

Diff Detail

Event Timeline

nhaustov updated this revision to Diff 48480.Feb 19 2016, 6:29 AM
nhaustov retitled this revision from to [AMDGPU] Assembler: Simplify handling of optional operands.
nhaustov updated this object.
nhaustov added reviewers: tstellarAMD, arsenm.
nhaustov set the repository for this revision to rL LLVM.
nhaustov added a project: Restricted Project.
nhaustov added a subscriber: SamWot.
nhaustov updated this revision to Diff 48483.Feb 19 2016, 6:32 AM
nhaustov removed rL LLVM as the repository for this revision.

Remove comments

tstellarAMD accepted this revision.Feb 19 2016, 4:36 PM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Feb 19 2016, 4:36 PM
nhaustov closed this revision.Feb 24 2016, 6:28 AM

Excuse me, I have reverted this in r261839.

Valgrind log:

******************** TEST 'LLVM :: MC/AMDGPU/flat.s' FAILED ********************
Script:
--
bin/llvm-mc -arch=amdgcn -mcpu=bonaire -show-encoding llvm/test/MC/AMDGPU/flat.s | bin/FileCheck llvm/test/MC/AMDGPU/flat.s --check-prefix=CIVI --check-prefix=CI
not bin/llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding llvm/test/MC/AMDGPU/flat.s | bin/FileCheck llvm/test/MC/AMDGPU/flat.s --check-prefix=CIVI --check-prefix=VI
not bin/llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding llvm/test/MC/AMDGPU/flat.s 2>&1 | bin/FileCheck llvm/test/MC/AMDGPU/flat.s --check-prefix=NOVI
not bin/llvm-mc -arch=amdgcn -show-encoding llvm/test/MC/AMDGPU/flat.s 2>&1 | bin/FileCheck llvm/test/MC/AMDGPU/flat.s --check-prefix=NOSI
not bin/llvm-mc -arch=amdgcn -mcpu=SI -show-encoding llvm/test/MC/AMDGPU/flat.s 2>&1 | bin/FileCheck llvm/test/MC/AMDGPU/flat.s --check-prefix=NOSI
--
Exit Code: 123

Command Output (stderr):
--
==8345== Conditional jump or move depends on uninitialised value(s)
==8345==    at 0x98DF6E: llvm::AMDGPUInstPrinter::printSLC(llvm::MCInst const*, unsigned int, llvm::raw_ostream&) (AMDGPUInstPrinter.cpp:121)
==8345==    by 0x9916F0: llvm::AMDGPUInstPrinter::printInstruction(llvm::MCInst const*, llvm::raw_ostream&) (AMDGPUGenAsmWriter.inc:18576)
==8345==    by 0x98DA23: llvm::AMDGPUInstPrinter::printInst(llvm::MCInst const*, llvm::raw_ostream&, llvm::StringRef, llvm::MCSubtargetInfo const&) (AMDGPUInstPrinter.cpp:26)
==8345==    by 0x7FB178: llvm::MCTargetStreamer::prettyPrintAsm(llvm::MCInstPrinter&, llvm::raw_ostream&, llvm::MCInst const&, llvm::MCSubtargetInfo const&) (MCStreamer.cpp:640)
==8345==    by 0x7AB90B: (anonymous namespace)::MCAsmStreamer::EmitInstruction(llvm::MCInst const&, llvm::MCSubtargetInfo const&) (MCAsmStreamer.cpp:1449)
==8345==    by 0x4668B4: (anonymous namespace)::AMDGPUAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned int&, llvm::SmallVectorImpl<std::unique_ptr<llvm::MCParsedAsmOperand, std::default_delete<llvm::MCParsedAsmOperand> > >&, llvm::MCStreamer&, unsigned long&, bool) (AMDGPUAsmParser.cpp:664)
==8345==    by 0x82A6A6: (anonymous namespace)::AsmParser::parseStatement((anonymous namespace)::ParseStatementInfo&, llvm::MCAsmParserSemaCallback*) (AsmParser.cpp:1815)
==8345==    by 0x824B09: (anonymous namespace)::AsmParser::Run(bool, bool) (AsmParser.cpp:669)
==8345==    by 0x405AFF: AssembleInput(char const*, llvm::Target const*, llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo&, llvm::MCSubtargetInfo&, llvm::MCInstrInfo&, llvm::MCTargetOptions&) (llvm-mc.cpp:364)
==8345==    by 0x406C2A: main (llvm-mc.cpp:530)
==8345==
==8345== Conditional jump or move depends on uninitialised value(s)
==8345==    at 0x98DFBE: llvm::AMDGPUInstPrinter::printTFE(llvm::MCInst const*, unsigned int, llvm::raw_ostream&) (AMDGPUInstPrinter.cpp:127)
==8345==    by 0x99170C: llvm::AMDGPUInstPrinter::printInstruction(llvm::MCInst const*, llvm::raw_ostream&) (AMDGPUGenAsmWriter.inc:18577)
==8345==    by 0x98DA23: llvm::AMDGPUInstPrinter::printInst(llvm::MCInst const*, llvm::raw_ostream&, llvm::StringRef, llvm::MCSubtargetInfo const&) (AMDGPUInstPrinter.cpp:26)
==8345==    by 0x7FB178: llvm::MCTargetStreamer::prettyPrintAsm(llvm::MCInstPrinter&, llvm::raw_ostream&, llvm::MCInst const&, llvm::MCSubtargetInfo const&) (MCStreamer.cpp:640)
==8345==    by 0x7AB90B: (anonymous namespace)::MCAsmStreamer::EmitInstruction(llvm::MCInst const&, llvm::MCSubtargetInfo const&) (MCAsmStreamer.cpp:1449)
==8345==    by 0x4668B4: (anonymous namespace)::AMDGPUAsmParser::MatchAndEmitInstruction(llvm::SMLoc, unsigned int&, llvm::SmallVectorImpl<std::unique_ptr<llvm::MCParsedAsmOperand, std::default_delete<llvm::MCParsedAsmOperand> > >&, llvm::MCStreamer&, unsigned long&, bool) (AMDGPUAsmParser.cpp:664)
==8345==    by 0x82A6A6: (anonymous namespace)::AsmParser::parseStatement((anonymous namespace)::ParseStatementInfo&, llvm::MCAsmParserSemaCallback*) (AsmParser.cpp:1815)
==8345==    by 0x824B09: (anonymous namespace)::AsmParser::Run(bool, bool) (AsmParser.cpp:669)
==8345==    by 0x405AFF: AssembleInput(char const*, llvm::Target const*, llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo&, llvm::MCSubtargetInfo&, llvm::MCInstrInfo&, llvm::MCTargetOptions&) (llvm-mc.cpp:364)
==8345==    by 0x406C2A: main (llvm-mc.cpp:530)
==8345==

--
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1682

The UB can be appeased if

OptionalIdx[Op.getImmTy()] = i;

I'm not sure it were right.

Yes, the condition on token seems to be unnecessary. The map contains indexes in Operands vector, not target instruction.

nhaustov updated this revision to Diff 49025.Feb 25 2016, 2:46 AM
nhaustov edited edge metadata.