[PATCH] D78088: [MIR] Add comments to INLINEASM immediate flag MachineOperands

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

I like it!
Some minor comments and nits in line.



================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1330
+
+  if (MI.isInlineAsm()) {
+    std::string Flags;
----------------
Can we do an early exit here:

  if (!MI.isInlineAsm())
    return std::string();

that also reduces some identation below.. 


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1332
+    std::string Flags;
+    raw_string_ostream OS(Flags);
+
----------------
do we need `OS` here?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1338
+      if (ExtraInfo & InlineAsm::Extra_HasSideEffects)
+        OS << "sideeffect ";
+      if (ExtraInfo & InlineAsm::Extra_MayLoad)
----------------
can we then just do 

  return "sideeffect";

here, and similar for the if-stmts below?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1353
+      return OS.str();
+    } else {
+      int FlagIdx = MI.findInlineAsmFlagIdx(OpIdx);
----------------
We don't need the 'else'?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1369
+      case InlineAsm::Kind_Mem:                OS << "mem";       break;
+      default:                                 OS << "??";        break;
+      }
----------------
probably just an assert here is better?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1404
+        case InlineAsm::Constraint_Zy: OS << ":Zy"; break;
+        default: OS << ":?"; break;
+        }
----------------
and same here?


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:508
+
   // Only support immediates for now.
   if (Op.getType() != MachineOperand::MO_Immediate)
----------------
This comment is out of date now and needs a minor update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78088