- User Since
- Dec 17 2012, 10:03 AM (331 w, 2 d)
Mon, Apr 22
Thu, Apr 18
Could you please update the summary before pushing this?
Tue, Apr 16
- Use comma separated list instead of group naming.
Mon, Apr 15
The source change looks good to me, but I would like a bit more work on the test itself.
Thu, Apr 4
Looks reasonable to me.
Wed, Apr 3
Are the dead instructions marked during the detect dead lanes pass or during the rename independent SubReg pass?
The generic changes look sensible to me.
I would just suggest to add a comment on what VRM is for on the modified methods.
Wed, Mar 27
Tue, Mar 26
Mar 25 2019
Fix seg fault with PHIs
Mar 22 2019
Mar 21 2019
Mar 14 2019
All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.
Mar 13 2019
Landed in r356116.
Prototyped a retry legalization process in D59339.
The problem is that this patch is still needed to access the constant, because the legalization artifacts combiner inserts copies instead of updating the users.
For operands that truly need to be constants that shouldn't be legalized, I don't think G_CONSTANT should be used. This is part of the motivation behind D58232 https://reviews.llvm.org/D58232. In SelectionDAG there is the TargetConstant vs. Constant distinction, but I think that makes less sense here, so weird constants should just never be materialized.
Disclaimer: this is just my opinion and in particular I haven't talked with anybody about constants being place holders. So just see my comment as what they are: an individual throwing ideas :).
Update a mips test that I forgot.
Why wouldn't G_CONSTANT be legalized? This doesn't make any sense to me
Mar 12 2019
- Introduce a different variant of getConstantVRegVal instead of overriding the existing one (after talking to @aditya_nandakumar we decided we may want to have more control on what happens when we call this function).
- Add a few comments.
Mar 11 2019
I'm going to hold on that one for a little longer.
I wonder how SDAG handle constant during legalization, because I was not expecting that we would not issue the constant for the new type period.
I suspect it works because constants are just not materialized via instructions and maybe we should just ignore them during legalization. (If a constant is illegal, at the end of the legalization process, it should have any users, right?)
Mar 8 2019
Looks reasonable to me.
Feb 28 2019
Feb 25 2019
The LiveIntervals for the relevant regunits needs to be removed.
We should really just compute this property at some point (same for novregs), but that's for another day :).
LGTM. Nitpick below.
Feb 22 2019
LGTM, one last nitpick below.
Feb 19 2019
Feb 18 2019
Feb 14 2019
The generic part looks sensible to me.
LGTM, one nitpick below.
Feb 13 2019
Feb 12 2019
Feb 7 2019
Feb 6 2019
Thanks for the link @jfb.
- Switch to listing valid ordering instead of invalid ones
- Don't apply the transformation on volatile atomicrmw
Feb 5 2019
Though it's worth pointing out that none of those currently build a description of what it wants to do like the legalizer currently does, they just make the change straightaway when the rules match. Removing in LegalizationAction in favour of just making the change brings the legalizer closer to the others.
The legalizer is already done the way I'm talking about on the rule matching side and performance was marginally better on average than the old setAction() interface.
Should we revert this change to have the conversation going or am I the only one unhappy with the absence of guarantees for the other operands?
Personally, I'd like to replace the LegalizationAction with a function pointer that just performs the desired change and have a standard library of actions that can be called for the common changes.
Jan 24 2019
Thanks for your patience Matt!