[PATCH] D85966: [GlobalISel] Add a combine for sext_inreg(load x), c --> sextload x

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.
This revision is now accepted and ready to land.

LGTM with a couple nits on comments

(I can totally imagine typing "InReg" all of the time instead of "Inreg" and getting annoyed, but that may be a personal fault.)



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:618
 
+/// Match sext_inreg(load p), imm -> sextload p
+bool CombinerHelper::matchSextInregOfLoad(
----------------
I don't think we need Doxygen comments here?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:665
+
+  // Avoid widening the load at all.
+  auto &MMO = **LoadDef->memoperands_begin();
----------------
Comment is weird here?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:668
+
+  Builder.setInstrAndDebugLoc(MI);
+  auto *MF = MI.getParent()->getParent();
----------------
I don't think we do this everywhere in the combiner. Should we be doing this everywhere? (We should probably be doing this everywhere.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85966