[PATCH] D29011: [IR] Add Freeze instruction

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

Thanks, this looks about right.
I'm not sure about backend part of this patch, but that is likely okay.
Two main questions: should there be a constantexpr `freeze`, and should `freeze` be fully type-agnostic, like it is stated in D29121 <https://reviews.llvm.org/D29121>?



================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:487
+  bool translateFreeze(const User &U, MachineIRBuilder &MIRBuilder) {
+    return false;
+  }
----------------
aqjune wrote:
> lebedev.ri wrote:
> > Is this a correctness issue?
> > Or does returning `false` here results in IRTranslator "aborting"?
> Yep, returning false makes it abort, if my understanding is correct.
Okay.


================
Comment at: lib/AsmParser/LLParser.cpp:3407-3430
   // Unary Operators.
   case lltok::kw_fneg: {
     unsigned Opc = Lex.getUIntVal();
     Constant *Val;
     Lex.Lex();
     if (ParseToken(lltok::lparen, "expected '(' in unary constantexpr") ||
         ParseGlobalTypeAndValue(Val) ||
----------------
Should there be `constantexpr` version of `freeze`?


================
Comment at: lib/AsmParser/LLParser.cpp:6317-6338
 /// ParseUnaryOp
 ///  ::= UnaryOp TypeAndValue ',' Value
 ///
-/// If IsFP is false, then any integer operand is allowed, if it is true, any fp
-/// operand is allowed.
+/// If IsFP is true, then any fp operand is allowed.
+//  If IsInt is true, then any integer operand is allowed.
 bool LLParser::ParseUnaryOp(Instruction *&Inst, PerFunctionState &PFS,
+                            unsigned Opc, bool IsFP, bool IsInt) {
----------------
I see no restrictions on the type to `freeze` in D29121,
so i think you just don't want any checking for `freeze` here.
And existing `ParseUnaryOp` is only called with `/*IsFP*/true` to parse `fneg`.
So let's change this to
```
bool Valid = !IsFPOnly || LHS->getType()->isFPOrFPVectorTy();

```
?


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2448-2457
       case Instruction::FNeg: {
         assert(CE->getNumOperands() == 1 && "Unknown constant expr!");
         Code = bitc::CST_CODE_CE_UNOP;
         Record.push_back(getEncodedUnaryOpcode(CE->getOpcode()));
         Record.push_back(VE.getValueID(C->getOperand(0)));
         uint64_t Flags = getOptimizationFlags(CE);
         if (Flags != 0)
----------------
What about constantexpr `freeze`?


================
Comment at: lib/IR/Instructions.cpp:2240-2242
+    assert((getType()->isIntOrIntVectorTy() || getType()->isFPOrFPVectorTy()) &&
+           "Tried to create a freeze operation on a "
+           "non-integer, non-floating-point type!");
----------------
I'm not seeing this restriction in D29121


================
Comment at: lib/IR/Verifier.cpp:3141-3144
+  case Instruction::Freeze:
+    Assert(U.getType()->isIntOrIntVectorTy() || U.getType()->isFPOrFPVectorTy(),
+           "Freeze operator only works with float/int types!", &U);
+    break;
----------------
I'm not seeing this restriction in D29121


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

https://reviews.llvm.org/D29011