[PATCH] D53972: [ARM][CGP] Negative constant operand handling

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Had a first look, mostly nits.



================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:247
 /// Return whether the instruction can be promoted within any modifications to
 /// it's operands or result.
+bool ARMCodeGenPrepare::isSafeOverflow(Instruction *I) {
----------------
typo: it's -> its


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:481
   };
 
+  // First step is to prepare the instructions for mutation, as immediate
----------------
Thanks for writing this down! So please forgive my nitpicking here.


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:483
+  // First step is to prepare the instructions for mutation, as immediate
+  // values cause complications:
+  // - Most constants just need to be zero extended into their new type.
----------------
The ", as immediate values cause complications" comes a bit out of the blue for me. But I see what you want to say: we need to decide whether to sign or zero extend constants (as otherwise we miscompile).


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:484
+  // values cause complications:
+  // - Most constants just need to be zero extended into their new type.
+  // - For nuw binary operators, negative immediates could need sign extending.
----------------
Can you specify what you mean by "most constants"?


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:485
+  // - Most constants just need to be zero extended into their new type.
+  // - For nuw binary operators, negative immediates could need sign extending.
+  //   The operators that can wrap are: add, sub, mul and shl.
----------------
What exactly does "could" mean here? 


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:489
+  //     is an immediate, it will need zext to be nuw.
+  //   > I'm assuming mul couldn't be nuw while using a negative immediate...
+  //   > Which leaves the nuw add and sub to be handled; as with shl, if an
----------------
Same here: couldn't


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:499
+  // all the other immediates later.
+  for (auto *V : Visited) {
+    if (!isa<Instruction>(V))
----------------
This is more about style, so feel free to ignore, or see what you exactly want to do with this:
this function IRPromoter::Mutate is really big now. Perhaps (some) different things that we do here can be put in separate helper functions (like this below). I think it could help in making clearer the different "mutate steps" that are performed.


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:500
+  for (auto *V : Visited) {
+    if (!isa<Instruction>(V))
+      continue;
----------------
I think this worklist already contains instructions that we're interested in. I guess we don't need to check again.


https://reviews.llvm.org/D53972