User Details
- User Since
- Jul 4 2018, 10:36 AM (145 w, 6 d)
Sun, Apr 18
Update patch based on dependent D93370 patch.
- Rebase patch in order to update test cases
- Rebase patch
- Add documentation regarding refactored load and store implementation
- Add the PPC::MOF_SubtargetP10 flag if the subtarget has prefixed instructions
Wed, Apr 14
LGTM.
Tue, Apr 6
Address review comments:
- Update the comment for emitting pli directly
- Remove the case to ReplaceNode() if the resulting node from selectI64Imm() is not a constant.
I saw that the patch, along with the temporary workaround for clang-ppc64le-linux has been reverted. I also saw that you committed https://reviews.llvm.org/rGec575e3b0a462ff7a3d23d0f39a22147606050de without a work around for PPC, although that has also been reverted at this time.
Was your recent patch still problematic for PPC and would you still like us to investigate it?
Mon, Apr 5
Overall this LGTM.
Thu, Mar 25
- Moved setting FrameIndex alignment flags implementation to D93370 (with added comments from Nemanja's review)
- Create a static function to compute flags for address computation
- Create static function to set alignment flags for FrameIndex
- Update comments
Tue, Mar 23
LGTM.
This also LGTM.
Gentle ping.
Mar 19 2021
Mar 18 2021
Thanks for fixing the tests. I've tested this patch on the environment of clang-ppc64le-rhel (https://lab.llvm.org/buildbot/#/builders/57), and there seems to be two failures that still remain:
Failed Tests (2): Clang :: OpenMP/linking.c Clang :: Driver/msvc-link.c
Mar 15 2021
Update patch based on dependent patch (D95116) update, and update test cases with necessary changes.
Update patch based on dependent patch (D95115) update.
Update patch based on dependent patch (D93370), update newly added test cases, address review on comment updates.
Update patch based on dependent patch (D93370).
Rebase this patch to the latest changes.
This patch is NFC as it is just adding test cases. Update the revision to combine some of the files together and to reflect the final version that will be committed.
Mar 10 2021
Sorry for the delay and thank you for addressing my previous comments. I think this LGTM.
Mar 8 2021
I think this LGTM aside from the minor nit on the test case. I am OK with it being updated on the commit.
Mar 5 2021
Mar 4 2021
I believe this commit is causing failures on the following PowerPC buildbots:
https://lab.llvm.org/buildbot/#/builders/19/builds/2678
https://lab.llvm.org/buildbot/#/builders/18/builds/868
Feb 25 2021
Updated the patch to remove duplicate test cases.
Feb 24 2021
Feb 18 2021
Feb 17 2021
Just a few minor nit comments.
Feb 16 2021
Thanks for answering the questions I previously had on this patch.
In addition to the nit comments, I also have the same question as Stefan for getFPAs64BitIntHi/getFPAs64BitIntLo.
I just have a small question about the patch but overall I think this LGTM.
Commandeering this patch.
Feb 15 2021
Address clang-format comments.
Update patch based on dependent patches, and update a comment when checking for P10 specific flags.
Update patch based on dependent patch changes.
Update patch based on dependent patch (D93370).
Update patch to fix a small issue when setting the Base and Disp for DForms when we have constant that fits in 32-bits.
Previously I used a uint64_t when it should have been a uint16_t.
Feb 12 2021
Update patch to rebase from latest change on D93370.
Feb 11 2021
- Update LIT test cases with prefixed load/store checks.
- Added a PrefixInstrs flag.
- Rebase the patch with the latest trunk.
- Updated alignment checks for the frame index.
Update patch to move the check for if we have a value with an offset that fits into a 34-bit immediate into it's own condition instead of inside an else if.
Feb 9 2021
Feb 5 2021
Feb 4 2021
Feb 2 2021
Update patch to:
- rebase patch
- Update the naming of the variable to Parent
Update patch to rebase with latest trunk.
Update patch to rebase with latest trunk, and rearrange/clean up code slightly.
Jan 27 2021
Rebase patch and also add additional handling for frame index (if the frame index is not aligned, match to an XForm instruction).
Jan 22 2021
A few general comments.
Jan 21 2021
Also LGTM.
Jan 20 2021
Addressed review comments:
- Rename some of the MemOpFlags
- Fixed typo in comments
- Moved variables near their use
- Made some of the methods private
Jan 12 2021
Jan 7 2021
Removed the addition of isIntS32Immediate() - it is no longer needed after addressing the comment of using APInt.
- Updated names of the selection functions in the td patterns
- Address the comment of using APInt when computing address flags
- Removed NFC from the title as there is one test case update that we expect a DSForm in (instead of an XForm instruction)
Jan 6 2021
Thanks for updating. I just noticed one more minor nit.
Jan 4 2021
Dec 17 2020
LGTM. Thanks for the update Albion.
I just have a minor nit, but overall it looks good to me.
I'm really sorry for the delay. LGTM to me, as well.
Dec 16 2020
Add more comments to functions, make provablyDisjointOr() static, fix pattern in PPCInstr64Bit.td.
@steven.zhang The idea is that this patch should be NFC. All existing load/store test cases should pass with this refactoring. I do think there should be more tests added, perhaps in a follow up patch. What do you think?
Dec 15 2020
Please address the comments regarding the test cases.
Dec 4 2020
Just a minor comment but LGTM overall.
Dec 3 2020
Nov 12 2020
This LGTM.
Minor nit but LGTM
Overall LGTM.
Nov 11 2020
Please also address the clang-format comment.
Nov 10 2020
Aside from the clang-format, LGTM.
Thanks for fixing for tests and formatting. LGTM if there are no other concerns.