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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked an inline comment as done.
aditya_nandakumar added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:94
+                                     UseArg1 &&Arg1, UseArg2 &&Arg2) {
+    switch (Opc) {
+    case TargetOpcode::G_ADD:
----------------
Gerolf wrote:
> 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  ...
I hadn't thought about sorting. The order is mostly similar to the one in the DAG. I will alphabetically sort it here though.


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:126
+      return buildSExt(getDestFromArg(DstArg), getRegFromArg(Arg1));
+    }
+    auto MIB = buildInstr(Opc).addDef(getDestFromArg(DstArg));
----------------
Gerolf wrote:
> What about G_TRUNC?
I hadn't initially implemented all of them - as I thought I'd do that based on feedback. I'll certainly add G_TRUNC here.


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:200
+      return C1.ashr(C2);
+    case TargetOpcode::G_UDIV:
+      if (!C2.getBoolValue())
----------------
Gerolf wrote:
> BoolValue? Should that be an explicit check for zero instead?
That is how you check for zero in APInt.
  /// This converts the APInt to a boolean value as a test against zero.
  bool getBoolValue() const { return !!*this; }



================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:420
+                                     Op, MRI))
+    return buildConstant(Res, Val->getSExtValue());
   return buildInstr(TargetOpcode::G_ANYEXT).addDef(Res).addUse(Op);
----------------
Gerolf wrote:
> For AnyExt ConstandFoldUnaryOp() uses Zext, here you use Sext?
Will change to Zext here. Not sure if it matters for any_ext though.


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:439
 }
 
 MachineInstrBuilder MachineIRBuilder::buildSExtOrTrunc(unsigned Res,
----------------
Gerolf wrote:
> 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.
I hadn't hooked up G_TRUNC yet. Was planning to do it based on feedback.
I'll add it shortly.


https://reviews.llvm.org/D35594