[PATCH] D49512: [CodeGenPrepare] Add BothExtension type to PromotedInsts

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3295
+      ExtTy = BothExtension;
+    PromotedInsts[ExtOpnd] = TypeIsSExt(ExtOpnd->getType(), ExtTy);
+  }
----------------
When the type of the extension doesn't change, we should avoid updating the type, as the source type gives more information and is still relevant.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3307
+      return It->second.getPointer();
+    return nullptr;
+  }
----------------
I missed that part on my first read. Add a comment saying that if the extension doesn't match, we cannot use the information we had on the original type.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3600
-  PromotedInsts.insert(std::pair<Instruction *, TypeIsSExt>(
-      ExtOpnd, TypeIsSExt(ExtOpnd->getType(), IsSExt)));
   // Step #1.
----------------
Carrot wrote:
> qcolombet wrote:
> > Instead of insert, we should do update the value all the time.
> Function addPromotedInst exactly does this.
Oh sorry.

I missed it the first time, as I missed there was a new getOrigType function.
One thing that could help is a better documentation/name for the enum, e.g., instead of both something like stacked or instead of we saw both extension something that says one after the other.


================
Comment at: test/Transforms/CodeGenPrepare/X86/pr38125.ll:1
+; RUN: opt < %s -codegenprepare -S -mtriple=x86_64-unknown-unknown    | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
Please use a name for the filename that conveys what this test is checking without to have to check the PR.


================
Comment at: test/Transforms/CodeGenPrepare/X86/pr38125.ll:31
+  %z = zext i16 %t5 to i32
+  ret i32 %z
+}
----------------
Can't this be simplified further?
I don't think we would need more than one block for the problem at stake, right?


https://reviews.llvm.org/D49512