[PATCH] D150068: [X86][AsmParser] Refactor code in AsmParser

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3814
   case X86::INT: {
-    // Transforms "int $3" into "int3" as a size optimization.  We can't write an
-    // instalias with an immediate operand yet.
----------------
barannikov88 wrote:
> The comment was kind of useful. I was about to suggest adding InstAlias.
> 
Agreed. Just say "We can't write this as an InstAlias."


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstrOptimization.cpp:18
+using namespace llvm;
+bool X86::optimizeInstFromVEX3ToVEX2(MCInst &MI) {
+#define FROM_TO_RETURN(FROM, TO, NUM1, NUM2)                                   \
----------------
Adda blank line before this.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstrOptimization.cpp:19
+bool X86::optimizeInstFromVEX3ToVEX2(MCInst &MI) {
+#define FROM_TO_RETURN(FROM, TO, NUM1, NUM2)                                   \
+  case X86::FROM:                                                              \
----------------
Perhaps you could do something like

```
unsigned OpIdx1, OpIdx2;
unsigned NewOpc;
switch (Opc)
default:
  return true;
// Use macros to set OpIdx1, OpIdx2, NewOpc;
}

if (X86II::isX86_64ExtendedReg(MI.getOperand(OpIx1).getReg()) ||
    !X86II::isX86_64ExtendedReg(MI.getOperand(OpIdx2).getReg()))
  return false;
MI.setOpcode(New);
return true;
```

Then you're not duplicating the calls to isX86_64ExtendedReg and setOpcode in every macro?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstrOptimization.cpp:55
+bool X86::optimizeShiftRotateWithImmediateOne(MCInst &MI) {
+  auto E = MI.end();
+  if (MI.begin() == E)
----------------
Why bring iterators into this? Can't we use `getNumOperands() - 1`?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstrOptimization.cpp:62
+  case X86::FROM:                                                              \
+    if (!LastOp.isImm() || LastOp.getImm() != 1)                               \
+      return false;                                                            \
----------------
Do this outside the switch instead of duplicating for each case?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150068/new/

https://reviews.llvm.org/D150068