[PATCH] D29011: [IR] Add Freeze instruction

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:1715
+  /// the new value.
+  Value *CreateFreezeAtDef(Value *Arg, Function *F, const Twine &Name = "",
+                           bool replaceAllUses = true) {
----------------
aqjune wrote:
> filcab wrote:
> > This is not being used. Maybe submit in a later patch, with a use for it?
> Hello. Thanks for your comment.
> This function is going to be used in https://reviews.llvm.org/D29016, and this patch is important because it may reduce the number of unnecessarily inserted freezes.
That patch seems to have no instance of `CreateFreezeAtDef`. It's better to only add this function when it's going to be used.


================
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);
----------------
aqjune wrote:
> You're right - it is not used right now.
> However every other values has its own match function currently, so I just followed convention..
> If you think this is redundant now, I'll erase this.
It's a minor nit. I'm ok with this being added now with the rest of the instruction stuff.


================
Comment at: lib/Analysis/ValueTracking.cpp:3901
 
+bool llvm::isGuaranteedNotToBeUndefOrPoison(const Value *V) {
+  if (const Instruction *I = dyn_cast<Instruction>(V)) {
----------------
This function is unused.


================
Comment at: lib/Analysis/ValueTracking.cpp:3917
+    return false;
+  }
+
----------------
aqjune wrote:
> There's no plan to add other cases now.
> I added TODOs, as you recommended.
Maybe this?
```// TODO: Deal with other Constant subclasses.
if (isa<ConstantInt>(C))
  return true;

return false;```

I don't think the outer if is buying you anything for now. Anyone who adds more can deal with avoiding testing the same thing twice.
(we can even `return isa<...>...;`, but keeping the "itemization"-like code doesn't seem bad in my view).


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4350
+
+      I = new FreezeInst(Op);
+      InstructionList.push_back(I);
----------------
If you don't test for an integer type, this will generate invalid IR. It's also not consistent with the parser.


================
Comment at: lib/IR/Verifier.cpp:3654
 
+void Verifier::visitFreezeInst(FreezeInst &FI) {
+  Assert(FI.getOperand(0)->getType()->isIntegerTy(),
----------------
Please add a test to `unittests/IR/VerifierTest.cpp`


================
Comment at: test/Bitcode/freeze-vector.ll:3
+
+; CHECK: freeze-vector.ll:[[LINE:[0-9:]*]]: error: cannot freeze non-integer type
+define <2 x i32> @vec(<2 x i32> %x) {
----------------
No need to name the `LINE` variable. But you should be matching to make sure we get the correct line.
Something like (using the automatic `@LINE` variable):
```CHECK: freeze-vector.ll:[[@LINE+2]]: ```...
The same for the other tests.


https://reviews.llvm.org/D29011