[PATCH] D31676: [SelectionDAG] [ARM CodeGen] Fix chain information of LowerMUL

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi,

Thanks for this fix. I'm not 100% sure that all of it is needed - comments inline. Also, I particularly like the thoroughness of the commenting in your testcases. Much appreciated! :)

James



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7328
+    SDValue newLoad = SkipLoadExtensionForVMULL(LD, DAG);
+    DAG.ReplaceAllUsesOfValueWith(SDValue(LD, 1), newLoad.getValue(1));
+    unsigned Opcode = ISD::isSEXTLoad(LD) ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND;
----------------
This looks correct to me and looks like all you should need.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7329
+    DAG.ReplaceAllUsesOfValueWith(SDValue(LD, 1), newLoad.getValue(1));
+    unsigned Opcode = ISD::isSEXTLoad(LD) ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND;
+    SDValue extLoad =
----------------
From here below looks unnecessary and wrong.

It looks unnecessary because it's planting an extend of an extending load - which is surely pointless?

And it looks wrong because SkipLoadExtensionForVMULL has this comment:

  // We need to create a zextload/sextload. We cannot just create a load
  // followed by a zext/zext node because LowerMUL is also run during normal
  // operation legalization where we can't create illegal types.



Repository:
  rL LLVM

https://reviews.llvm.org/D31676