[PATCH] D88391: [M68k] (Patch 5/8) Target lowering

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:699
+      if (isAddressBase(N.getOperand(i))) {
+        return true;
+      }
----------------
RKSimon wrote:
> use llvm::any_of ?
I wish I could...but I don't think SDValue provides any iterator (or begin() / end() / ranges function) over its operands


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1490
+    }
+  }
+
----------------
RKSimon wrote:
> iirc we support this generically through a TLI callback. 
I assume you're talking about the lines above regarding converting mul to shift. If that's the case, are you suggesting. to put those into DAG combiner callbacks?


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1535
+  default:
+    llvm_unreachable("Unknown ovf instruction!");
+  case ISD::SADDO:
----------------
RKSimon wrote:
> won't this fire for smulo/umulo which you've commented out below?
currently umulo and smulo are legalized by expanding them, so they will not trigger this lowering function. 

I'm not sure we should use custom lowering for them though...it seemed like somebody tried before but failed with unknown reason and thus leave the code commented out here


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

https://reviews.llvm.org/D88391