[PATCH] D57041: [ARM][CGP] Check trunc value size before replacing

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Was just getting up to speed with this again; just a first round of nits before I start looking at the actual functional change.



================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:188
   return V->getType()->getScalarSizeInBits() <= ARMCodeGenPrepare::TypeSize;
 }
----------------
There's not enough context to show the class definition:

  namespace {
  class IRPromoter {
    SmallPtrSet<Value*, 8> NewInsts;
    SmallPtrSet<Instruction*, 4> InstsToRemove;
    DenseMap<Value*, SmallVector<Type*, 4>> TruncTysMap;
    SmallPtrSet<Value*, 8> Promoted;
    Module *M = nullptr;
    LLVMContext &Ctx;
    IntegerType *ExtTy = nullptr;
    IntegerType *OrigTy = nullptr;
    SmallPtrSetImpl<Value*> *Visited;
    SmallPtrSetImpl<Value*> *Sources;
    SmallPtrSetImpl<Instruction*> *Sinks;
    SmallPtrSetImpl<Instruction*> *SafeToPromote;
    ...

But I started looking at where `ExtTy` was defined again, because it was mentioned in the comments of this change. It is quite a crucial aspect of the IRPromoter, so perhaps you can spend a few comments on what it is and does.



================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:329
   // To demonstrate why we can't handle increasing values:
   // 
   // %add = add i8 %a, 2
----------------
nit: trailing whitespace


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:607
       continue;
   
     assert(EnableDSP && "DSP intrinisc insertion not enabled!");
----------------
nit: trailing whitespace here


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:703
 
-    // For any truncs that we insert to handle zexts, we can replace the
-    // result of the zext with the input to the trunc.
-    if (NewInsts.count(Src) && isa<ZExtInst>(V) && isa<TruncInst>(Src)) {
+    // Unless they produce a value that is narrower than ExtTy, we can
+    // we can replace the result of the zext with the input of a newly
----------------
typo: "we can we can"


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:703
 
-    // For any truncs that we insert to handle zexts, we can replace the
-    // result of the zext with the input to the trunc.
-    if (NewInsts.count(Src) && isa<ZExtInst>(V) && isa<TruncInst>(Src)) {
+    // Unless they produce a value that is narrower than ExtTy, we can
+    // we can replace the result of the zext with the input of a newly
----------------
SjoerdMeijer wrote:
> typo: "we can we can"
nit: they = zexts?


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:705
+    // we can replace the result of the zext with the input of a newly
+    // inserted the trunc.
+    if (NewInsts.count(Src) && isa<TruncInst>(Src) && EqualTypeSize(Src)) {
----------------
typo: remove "the"?


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

https://reviews.llvm.org/D57041