Page MenuHomePhabricator

[X86] Add constant pool decoding to more instructions
Needs ReviewPublic

Authored by craig.topper on Aug 26 2017, 1:17 PM.

Details

Summary

This is a proof of concept to add constant pool decoding to more instructions.

So far I've only added support for add/sub/and/or/xor. I've printed the and/or/xor as bytes, since it was easier to think about small immediates. Maybe we should print them in hex?

I only regenerated a few tests just as as a demonstration.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 26 2017, 1:17 PM
chandlerc edited edge metadata.Aug 26 2017, 4:08 PM

Generally speaking, I love this kind of thing. I think it makes tests and the actual assembly dramatically easier to read.

Some comments:

  1. I think we should just print hex everywhere, always. I keep trying to think of why we *shouldn't* print in hex, and I've got nothing...
  2. I don't feel strongly about byte vs. other granualarity for AND/OR/XOR... Maybe byte-digit-separated single integer? So, 0xFF'FF'FF'FF'FF'FF.... Dunno, don't feel strongly about any of this
  3. We should probably change the movdqa and such printing to use the same code for formatting the integers?
spatel added inline comments.Aug 27 2017, 8:22 AM
lib/Target/X86/X86MCInstLower.cpp
1406–1415

Can you detect a splat here and print something like "[0x1234 splat]"? That would greatly improve readability in the common case IMO, and make it clear in the odd case when the constant is not a splat.

RKSimon edited edge metadata.Sep 4 2017, 10:01 AM
  1. I think we should just print hex everywhere, always. I keep trying to think of why we *shouldn't* print in hex, and I've got nothing...

+1 Definitely hex, not sure whether we should use leading zeros as well - will bulk things but will also makes things consistent.

  1. I don't feel strongly about byte vs. other granualarity for AND/OR/XOR... Maybe byte-digit-separated single integer? So, 0xFF'FF'FF'FF'FF'FF.... Dunno, don't feel strongly about any of this

No preference here, just not too dissimilar to what is being done everywhere else. Bytes are probably the most readable?

  1. We should probably change the movdqa and such printing to use the same code for formatting the integers?

+1 Although not sure if it'll cause a massive number of test changes?

lib/Target/X86/X86MCInstLower.cpp
1406–1415

This may work especially well for avx512 broadcast source instructions?

1951

Add a TODO for other instructions (CMPs, MULs + SHIFTSs come to mind)?

What's happening with this? From D44345 I guess you're still interested in this kind of thing?

Switch to hex. Support splats. Print and/or/xor as 64 bit because byte size interferes with splat recognizition.

I also picked up some broadcast and fma constant changes in here. I'll separate those out before commit.

This is a good improvement, but can we make the splats even less of an eye chart (let me know if you see any problems with this):

Index: lib/Target/X86/X86MCInstLower.cpp
===================================================================
--- lib/Target/X86/X86MCInstLower.cpp (revision 327978)
+++ lib/Target/X86/X86MCInstLower.cpp (working copy)
@@ -1447,6 +1447,81 @@
   }
 }

+static void printConstantPoolOp(MCStreamer &OutStreamer,
+                                const MachineInstr *MI,
+                                StringRef Op, unsigned EltSize,
+                                bool EOL) {
+  if (!OutStreamer.isVerboseAsm())
+    return;
+
+  const MCInstrDesc &Desc = MI->getDesc();
+  int MemOperand = X86II::getOperandBias(Desc) +
+                   X86II::getMemoryOperandNo(Desc.TSFlags);
+  const MachineOperand &MemOp = MI->getOperand(MemOperand + 3);
+
+  auto *C = getConstantFromPool(*MI, MemOp);
+  if (!C)
+    return;
+
+  SmallVector<uint64_t, 16> Vec;
+  APInt UndefElts;
+  if (!extractConstantMask(C, EltSize, UndefElts, Vec))
+    return;
+
+  const MachineOperand &DstOp = MI->getOperand(0);
+  const MachineOperand &SrcOp = MI->getOperand(Desc.getNumDefs());
+
+  std::string Comment;
+  raw_string_ostream CS(Comment);
+
+  CS << X86ATTInstPrinter::getRegisterName(DstOp.getReg());
+  CS << " = ";
+  CS << X86ATTInstPrinter::getRegisterName(SrcOp.getReg());
+  CS << Op << "[";
+
+  // Try to print the minimal splat constant. The vector constant returned by
+  // extractConstantMask may have wider elements than the element type of the
+  // actual mask constant.
+  Type *CTy = C->getType();
+  unsigned VecNumElts = Vec.size();
+  if (CTy->isVectorTy() && CTy->getVectorNumElements() > VecNumElts) {
+    if (auto *SplatC = dyn_cast_or_null<ConstantInt>(C->getSplatValue())) {
+      uint64_t SplatVal = SplatC->getZExtValue();
+      CS << format_hex(SplatVal, CTy->getScalarSizeInBits() / 4 + 2);
+      CS << " splat]";
+      OutStreamer.AddComment(CS.str(), EOL);
+      return;
+    }
+  }
+
+  // Print the mask
+  bool IsSplat = true;
+  for (unsigned i = 1; i < VecNumElts; ++i) {
+    if (Vec[0] != Vec[i]) {
+      IsSplat = false;
+      break;
+    }
+  }
+
+  for (unsigned i = 0; i < VecNumElts; ++i) {
+    if (i != 0)
+      CS << ",";
+    if (UndefElts[i])
+      CS << 'u';
+    else
+      CS << format_hex(Vec[i], EltSize / 4 + 2);
+
+    if (IsSplat) {
+      CS << " splat";
+      break;
+    }
+  }
+
+  CS << "]";
+
+  OutStreamer.AddComment(CS.str(), EOL);
+}
+
 void X86AsmPrinter::EmitInstruction(const MachineInstr *MI) {
   X86MCInstLower MCInstLowering(*MF, *this);
   const X86RegisterInfo *RI = MF->getSubtarget<X86Subtarget>().getRegisterInfo();