[PATCH] D47568: [Power9] Do the add-imm peephole for pseudo instruction DFLOADf32/DFLOADf64 and the store pair

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I'd typically be OK with these cosmetic changes being made on the commit, but there are a large enough number of changes that I'd prefer to see the updated patch. Thanks for fixing these.



================
Comment at: llvm/test/CodeGen/PowerPC/toc-float.ll:1
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 <%s | FileCheck -check-prefix=CHECK-P9 %s
+
----------------
Nit: There's only one RUN in this test case, no need for the check prefix - just use the default `CHECK` directives with no prefix. Also, it would be good to have all the following tests:
- Returning a `double` constant that can be represented as `float` (you already have this)
- Returning a `double` constant that cannot be represented as `float` (you already have this)
- Returning a `float` constant
- Accessing a global array of `float` (you already have this)
- Accessing a global array of `double`
- Accessing a global array of either `double` or `float` where the index of the access is large enough that a `D-Form` load cannot be used (perhaps above `4096` for `double`)


================
Comment at: llvm/test/CodeGen/PowerPC/toc-float.ll:4
+define double @bar() {
+ret double 1.400000e+01
+}
----------------
Nit: indentation.


================
Comment at: llvm/test/CodeGen/PowerPC/toc-float.ll:12
+define double @foo() {
+ret double 1.400004e+01
+}
----------------
Nit: please change the constant being returned to be significantly different from the one being returned from `bar()` so that it is obvious from a quick visual inspection that they're different. Also, you should add a comment that the constant cannot be represented exactly as `float` so a `double` is loaded from the constant pool. This can either be with a comment or by naming the function adequately.


https://reviews.llvm.org/D47568