[PATCH] D29011: [IR] Add Freeze instruction

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Just some minor nits.
You're adding some functions which will never be called. It might be easier to hold them off until the next patch (and have this one only add the instruction + necessary plumbing), or to at least post a follow-up patch on phabricator which shows usage for those functions, as that makes it easier to know it shouldn't be dead code for long.



================
Comment at: include/llvm/IR/IRBuilder.h:1713
 
+  /// \brief Insert a freeze instruction at the definition of \p Arg and return 
+  /// the new value.
----------------
Maybe omit the `\brief`?


================
Comment at: include/llvm/IR/IRBuilder.h:1715
+  /// the new value.
+  Value *CreateFreezeAtDef(Value *Arg, Function *F, const Twine &Name = "",
+                           bool replaceAllUses = true) {
----------------
This is not being used. Maybe submit in a later patch, with a use for it?


================
Comment at: include/llvm/IR/Instructions.h:5064
+/// This class represents a freeze function that returns random concrete
+/// value if an operand is either a poison value or an undefine value
+class FreezeInst : public UnaryInstruction {
----------------
`undef`?


================
Comment at: include/llvm/IR/PatternMatch.h:931
+template <typename OpTy>
+inline FreezeClass_match<OpTy> m_Freeze(const OpTy &Op) {
+  return FreezeClass_match<OpTy>(Op);
----------------
Minor nit: Unsure if the usual pattern matching policy is to add the pattern only when you need it on when adding the instruction.


================
Comment at: lib/Analysis/ValueTracking.cpp:3917
+    return false;
+  }
+
----------------
If we're just doing `ConstantInt` for now, why not just use the inner if (same for the `FreezeInst`)? Then change it after adding more, if the code ends up being too complex or has too many repeated tests.
No need to add a comment stating what an `if (isa...) return` is doing.
Is there a plan for adding constants, etc? Maybe add a TODO tag here, so it jumps out if someone is grepping for those.


================
Comment at: lib/AsmParser/LLParser.cpp:5914
+  if (!Op->getType()->isIntegerTy())
+    return Error(Loc, "cannot freeze non-integer type");
+
----------------
Please test this error.


================
Comment at: lib/AsmParser/LLParser.cpp:5916
+
+  Inst = new FreezeInst(Op, "");
+  return false;
----------------
Omit the `, ""` since it's the default arg anyway.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4350
+
+      I = new FreezeInst(Op, "");
+      InstructionList.push_back(I);
----------------
Omit the `, ""`.


https://reviews.llvm.org/D29011