- User Since
- Sep 6 2019, 10:50 AM (30 w, 4 d)
This patch now implements a bit more than the original commit message would suggest. Could you please update? Thanks for working on this!
Fri, Apr 3
Btw, could you also add some negative tests? (e.g. out-of-range immediate)
Thu, Apr 2
Cheers for working on this @kmclaughlin !
Tue, Mar 24
Cheers for the fix @sdesmalen !
Mon, Mar 23
Thanks for the updates @kmclaughlin ! Would you mind adding a comment to clearly mark the negative tests? E.g. for @reinterpret_reductions_1? Maybe also a comment why a particular case is not optimised? You've already done that for some tests, but not all of them.
Fri, Mar 20
Have you considered adding an interface for the new PM?
Thu, Mar 19
Wed, Mar 18
Cheers for working on this, LGTM!
Tue, Mar 17
Mon, Mar 16
Cheers for working on this @kmclaughlin ! Have you considered adding an interface for the new PM? You could check this for reference: https://reviews.llvm.org/rGd6de5f12d485a85504bc99d384a85634574a27e2 (also implements a FunctionPass).
Fri, Mar 13
Thanks you for addressing my comments @fpetrogalli !
Thu, Mar 12
- Renamed @llvm.aarch64.sve.dup.unpred as @llvm.aarch64.sve.dup.x (this is consistent with other unpredicated intrinsics)
- Restored the original name of @llvm.aarch64.sve.dup
Wed, Mar 11
Rename getScaledOffsetForLDNT1 as getScaledOffsetForBitwidth
Tue, Mar 10
@fpetrogalli Thanks for the update - this has evolved into a very neat patch! I've left a few nits, nothing major. One additional nice-to-have: could the commit message contain the list of intrinsics that you are adding? Personally I think that making references to ACLE there is not needed, but I don't mind it being there.
Fix formatting to appease Harbormaster.
Mar 6 2020
- Replaced ISD::MUL from my original patch with ISD::SHL
- Added patterns for ISD::SHL for scalable types
- Extracted the scaling into a seperate function
Mar 5 2020
Mar 4 2020
Just a quick note: this patch is formatted consistently with the files that it touches. Since these files diverge from the official clang-format rules, clang-format pre-merge checks failed.
Hi @fpetrogalli, thank you for working on this!
LGTM, cheers for working on this!
Mar 2 2020
Feb 27 2020
Feb 26 2020
@sdesmalen Thank you for reviewing! Yes, the tests were failing with expensive checks enabled. Great catch, thank you!
Feb 25 2020
This is almost literally copy & paste of the implementation of the regular gather loads, hence submitting as one patch (there are only few meaningful changes). I generated the tests by copying the corresponding tests for regular gather loads (I did update the opcodes though).
I've already updated the summary and replied to comments inline. Here's a summary of the latest changes:
Thank you for taking a look @sdesmalen!
I removed the functional changes from AArch64ISelLowering.cpp.
Feb 24 2020
Feb 21 2020
Fixing typo: STD --> SDT.
@fpetrogalli Thank you for working on this! A few nits, but nothing blocking. LGTM!
That's a good point, thank you! I've updated the summary to reflect that.
Cheers for working on this @kmclaughlin!
Feb 20 2020
I've made some NFC changes, so that we re-use more code.
Hi @efriedma , thank you for taking a look!
Feb 19 2020
Thank you for all the updates @fpetrogalli ! LGTM
Feb 17 2020
Cheers for updating this @fpetrogalli ! My most recent comments are mostly nits.
Feb 13 2020
Feb 12 2020
Hi @fpetrogalli, thank you for working on this.
Feb 11 2020
Jan 28 2020
One [nit] (non-blocking), otherwise LGTM.
Jan 27 2020
A few nits. Also, in the commit msg:
Jan 25 2020
Jan 20 2020
Jan 17 2020
Revert previous patch and settle on llvm.aarch64.sve.ld1.gather.scalar.offset
Jan 16 2020
Fix the scaling of the offset
@fpetrogalli Thank you for working on this - just a few nits from me!
- Fix the offset limit in GLD1_IMM and SST1_IMM (now it depends on the size of the underylying datatype)
- Update the tests accordingly
Jan 15 2020
Similarly to gather loads, I've updated intrinisics for scatter stores ("vector base, scalar offset").
- Handled casses when the immediate offset is out of range (add relevant tests too)
- Tweaked comments to better reflect the distinction that the ACLE makes ("scalar base, vector offsets" vs "vector base, scalar offset")
- Updated commit msg
Jan 14 2020
- Reverted changes from the previous patch
- Renamed int_aarch64_sve_ld1_gather_imm to int_aarch64_sve_ld1_gather_scalar_offset
- Renamed sve-intrinsics-gather-loads-vector-base.ll as sve-intrinsics-gather-loads-vector-base-imm-offset.ll
- Added DAG combine rule for GLD1_IMM for scenarios where the offset is a non-immediate scalar
- Added sve-intrinsics-gather-loads-vector-base-scalar-offset.ll test file
Jan 3 2020
Jan 2 2020
Thank you for taking a look @eli.friedman. I've kept int_aarch64_sve_ld1_gather and int_aarch64_sve_ld1_gather_imm separate to better reflect the difference in the semantics of the corresponding instructions. I think that merging the two would obfuscate that separation a bit (though reduce code, which is usually a good thing). Keeping it as is would be my preference, but I might be missing the bigger picture?