[PATCH] D29011: [IR] Add Freeze instruction

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri edited reviewers, added: lebedev.ri, nlopes, jdoerfert, regehr, filcab, delcypher; removed: chandlerc, majnemer, sanjoy, sunfish, espindola.
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: hiraditya.

Thank you for working on this! This needs more cleanup.
Since the patch was initially posted, and Unary operator type was added to LLVM.
In last patch's update many parts of the patch were correctly migrated, but not all.
See D53877 <https://reviews.llvm.org/D53877> for the list of potential places, i tried to point out most of them inline.



================
Comment at: include/llvm-c/Core.h:144
   LLVMCleanupPad     = 64,
-  LLVMCatchSwitch    = 65
+  LLVMCatchSwitch    = 65,
 } LLVMOpcode;
----------------
unrelated


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:394-395
 enum UnaryOpcodes {
   UNOP_NEG = 0
 };
 
----------------
Did you mean to put `freeze` as an unary instruction instead of a function?


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:559
   FUNC_CODE_OPERAND_BUNDLE = 55, // OPERAND_BUNDLE: [tag#, value...]
   FUNC_CODE_INST_UNOP = 56,      // UNOP:       [opcode, ty, opval]
   FUNC_CODE_INST_CALLBR = 57,    // CALLBR:     [attr, cc, norm, transfs,
----------------
Wouldn't basing it on unary op work?


================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:487
+  bool translateFreeze(const User &U, MachineIRBuilder &MIRBuilder) {
+    return false;
+  }
----------------
Is this a correctness issue?
Or does returning `false` here results in IRTranslator "aborting"?


================
Comment at: include/llvm/IR/IRBuilder.h:2358
+  Value *CreateFreeze(Value *V, const Twine &Name = "") {
+    return Insert(new FreezeInst(V), Name);
+  }
----------------
I personally have mild preference for the `::Create`; not sure if this is a remark or review note.


================
Comment at: include/llvm/IR/InstVisitor.h:202
   RetTy visitCatchPadInst(CatchPadInst &I)     { DELEGATE(FuncletPadInst); }
+  RetTy visitFreezeInst(FreezeInst &I)         { DELEGATE(Instruction); }
 
----------------
Should you delegate to `UnaryInstruction` instead?


================
Comment at: lib/AsmParser/LLParser.cpp:6739
+
+  if (!Op->getType()->isIntegerTy())
+    return Error(Loc,"cannot freeze non-integer type");
----------------
Needs updating to match langref - vectors; floating point support?


================
Comment at: lib/IR/Verifier.cpp:3964-3965
+void Verifier::visitFreezeInst(FreezeInst &FI) {
+  Assert(FI.getOperand(0)->getType()->isIntegerTy(),
+         "Cannot freeze non-integer type!", &FI);
+
----------------
Needs updating to match langref - vectors; floating point support?


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

https://reviews.llvm.org/D29011