This is an archive of the discontinued LLVM Phabricator instance.

[X86] Implementation of X86Operand::print
ClosedPublic

Authored by avt77 on Dec 28 2017, 12:45 AM.

Details

Summary

The current body of the feature is empty. This patch produces the real info describing the given MC operand. Such extended debug logging realy helped when I was working with D41595.

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Dec 28 2017, 12:45 AM
RKSimon added inline comments.Dec 29 2017, 2:07 PM
lib/Target/X86/AsmParser/X86Operand.h
115 ↗(On Diff #128270)

If possible, I'd make this a lambda function inside the print function below and hopefully remove the need for the union.

avt77 updated this revision to Diff 129067.Jan 9 2018, 5:35 AM

The implemetation was switched to lambda function.

A couple of minors, but no more comments from me. @craig.topper ?

lib/Target/X86/AsmParser/X86Operand.h
102 ↗(On Diff #129067)

Capitalization: PrintImmValue

107 ↗(On Diff #129067)

Add an assert message

avt77 added inline comments.Jan 9 2018, 8:30 AM
lib/Target/X86/AsmParser/X86Operand.h
107 ↗(On Diff #129067)

Maybe it's better something like (instead of assertion):

 if (const MCSymbolRefExpr &SRE = dyn_cast<MCSymbolRefExpr>(*Val)) {
   const MCSymbol &Sym = SRE.getSymbol();
   if (auto SymName = Sym.getName().data())
......

to be sure it can't raise exception?

avt77 updated this revision to Diff 129111.Jan 9 2018, 9:40 AM

I renamed the feature and replaced the assertion with dyn_cast.

craig.topper added inline comments.Jan 9 2018, 10:42 AM
lib/Target/X86/AsmParser/X86Operand.h
39 ↗(On Diff #129111)

Why isn't this part of the RegOp struct? Though I suggested a way we can remove this entirely further down.

121 ↗(On Diff #129111)

It would print lower case instead of upper case, but I think you can use X86IntelInstPrinter::getRegisterName which is a static method and then you wouldn't have to pass MRI around.

avt77 updated this revision to Diff 129236.Jan 10 2018, 3:51 AM

I removed usage of MRI thanking to Craig's suggest.

This revision is now accepted and ready to land.Jan 10 2018, 11:30 PM
This revision was automatically updated to reflect the committed changes.