[PATCH] D35594: [GISel]: ConstantFold operations when building MIR

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:94
+                                     UseArg1 &&Arg1, UseArg2 &&Arg2) {
+    switch (Opc) {
+    case TargetOpcode::G_ADD:
----------------
How do you sort the opcodes? The order here is different from other functions  (ConstantFoldBinop) in your code? Why not be consistent and eg. order all arithmetic (alphabetical), then all logical (alphabetical) or all alphabetical or  ...


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:126
+      return buildSExt(getDestFromArg(DstArg), getRegFromArg(Arg1));
+    }
+    auto MIB = buildInstr(Opc).addDef(getDestFromArg(DstArg));
----------------
What about G_TRUNC?


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:200
+      return C1.ashr(C2);
+    case TargetOpcode::G_UDIV:
+      if (!C2.getBoolValue())
----------------
BoolValue? Should that be an explicit check for zero instead?


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:420
+                                     Op, MRI))
+    return buildConstant(Res, Val->getSExtValue());
   return buildInstr(TargetOpcode::G_ANYEXT).addDef(Res).addUse(Op);
----------------
For AnyExt ConstandFoldUnaryOp() uses Zext, here you use Sext?


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:439
 }
 
 MachineInstrBuilder MachineIRBuilder::buildSExtOrTrunc(unsigned Res,
----------------
Since ConstandFoldUnaryOp supports 4 opcodes, I had expected four calls to it in functions buildSExt, buildZext, buildAnyExt and *buildTrunc*. But the last one seems missing.  If the function below (buildSExtorTrunc is covering that than I miss a call to ConstandFoldUnaryOp() there. So either way the code doesn't look quite kosher.


https://reviews.llvm.org/D35594