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

Gerolf Hoflehner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 27 00:18:21 PDT 2017

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.


More information about the llvm-commits mailing list