This revision was automatically updated to reflect the committed changes.
Closed by commit rGe29bb074c62c: [PowerPC] Exploit xxsplti32dx (constant
materialization) for scalars (authored by Conanap).
Changed prior to commit:
https://reviews.llvm.org/D95458?vs=329015=333101#toc
Repository:
rG
stefanp accepted this revision.
stefanp added a comment.
This revision is now accepted and ready to land.
Thank you for adding this!
Other than one minor nit I think this LGTM.
Feel free to address nits on commits.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8834
amyk added inline comments.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16134
if (Subtarget.hasPrefixInstrs()) {
- // With prefixed instructions, we can materialize anything that can be
- // represented with a 32-bit immediate, not just positive zero.
Conanap updated this revision to Diff 329015.
Conanap marked 3 inline comments as done.
Conanap added a comment.
Updated some comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95458/new/
https://reviews.llvm.org/D95458
Files:
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
amyk added inline comments.
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:1885
+let isReMaterializable = 1, isMoveImm = 1, Predicates = [PrefixInstrs] in {
+ def XXSPLTI32DX :
I think it might be good to add a comment of why the `XXSPLTI32DX`
Conanap marked 2 inline comments as done.
Conanap added inline comments.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8593
+ return !convertToNonDenormSingle(ArgAPFloat);
+}
+
stefanp wrote:
> I'm wondering if it would not be better to just inline
Conanap updated this revision to Diff 327537.
Conanap added a comment.
Addressed Stefan's comments, converted the check to a mirror of the original
function for XXSPLTIDP except non-destructive.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95458/new/
https://reviews.llvm.org/D95458
stefanp added a comment.
Comments relate to just cleaning up the patch a little.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8593
+ return !convertToNonDenormSingle(ArgAPFloat);
+}
+
I'm wondering if it would not be better to just inline this.
Conanap added inline comments.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:1321
bool convertToNonDenormSingle(APFloat );
+ bool checkNonDenormCannotConvertToSingle(APInt );
+ bool checkNonDenormCannotConvertToSingle(APFloat );
stefanp wrote:
> Is
Conanap updated this revision to Diff 326000.
Conanap marked 6 inline comments as done.
Conanap added a comment.
Addressed some nits and a problem where sometimes the compiler would spill half
way through materialization.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95458/new/
amyk added a comment.
In addition to the nit comments, I also have the same question as Stefan for
`getFPAs64BitIntHi`/`getFPAs64BitIntLo`.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8591
+ // Only convert if it loses info, since XXSPLTIDP should
+ // handle
stefanp requested changes to this revision.
stefanp added inline comments.
This revision now requires changes to proceed.
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:1321
bool convertToNonDenormSingle(APFloat );
+ bool checkNonDenormCannotConvertToSingle(APInt );
+
Conanap updated this revision to Diff 321184.
Conanap added a comment.
Updated to ensure the shortcircuit protects against the destructive function.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95458/new/
https://reviews.llvm.org/D95458
Files:
Conanap created this revision.
Conanap added reviewers: nemanjai, saghir, PowerPC.
Conanap added projects: LLVM, clang, PowerPC.
Herald added a subscriber: kbarton.
Conanap requested review of this revision.
Previously related differential (exploit xxsplti32dx for vectors) here:
14 matches
Mail list logo
