Compilers at a fruit company
- User Since
- Sep 9 2013, 3:45 AM (280 w, 2 d)
This should be abandoned?
Fri, Jan 18
Tue, Jan 15
Just checking, this is dependent on D55639?
This is a good point, so I think that if we enable it for -O0 it should only be for constants.
I've also updated most tests (except some tests where it's not obvious what the test is doing where CSE is disabled).
If CSE should be enabled by default, I guess that we'll need to figure out what the tests are supposed to be doing so that there aren't many tests mainly testing non-default behaviour?
Mon, Jan 14
Thu, Jan 10
Tue, Jan 8
Mon, Jan 7
This is strange. G_CONSTANT isn't supposed to be able to splat the constant into a vector, so this is an oversight in the MachineVerifier for not catching this case.
Thanks for the cleanup. LGTM with minor naming change.
Dec 20 2018
LGTM with minor change.
Dec 19 2018
LGTM. In general for IR translator tests we don't need to check for every type combination if it's a simple 1-1 mapping, but no need to change the patch.
Dec 13 2018
I'd like to revive this patch, as the other IR canonicalization pass is much more controversial.
Dec 10 2018
Thanks, committed in r348788.
Dec 7 2018
The patch title should probably say adding IR translation support instead of selection support. If we're adding selection support too then we should have a test case, otherwise it can be in a later patch.
Dec 5 2018
Dec 4 2018
This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.
Dec 3 2018
Updated to incorporate Volkan's patches to change buildMerge calls to explicitly check for types before choosing the right build method. Also added test for and fixed artifact combiner for G_BUILD_VECTOR.
Nov 30 2018
Addressed comments, deleted the non power of 2 test.
Changed the approach to replace values in PendingLoads in place. Also added an assert in SelectionDAG::createOperands() to check if we can store the requested number of operands.
Nov 29 2018
Nov 28 2018
Nov 15 2018
So these extends are created by the legalizer, correct? If so, it seems ok to me as that's what these artifact combines were intended for.
Nov 5 2018
Added comment for G_BUILD_VECTOR_TRUNC and tests for the verifier. I haven't added all the different combinations because we can only write one test per file, so I tested the most useful of the verifier errors.
Updated patch to introduce a separate G_BUILD_VECTOR_TRUNC opcode.
Nov 2 2018
As long as <4 x s32> = the G_BUILD_VECTOR could be legalized I think the worst that will happen is that we just get poor codegen.
Also, just throwing that here, I haven't really thought about it: should we rethink our legality model to check patterns instead of one instruction at a time?
I.e., maybea(s8) = ...
is not legal buta(s8) = ... b(<4 x s8>) = build_vector a(s8)
I think the issue of sequences becoming illegal because of things like copies being introduced is still a deal breaker for that, and it would also introduce a code motion barrier since we can't select sequences spanning blocks. E.g. it wouldn't be safe to move the insn in the predecessor due to other uses.
This actually ties back to one question that I don't think we answered. Do we legalize on the type (i.e., s8 to s32) or the size and type can be different (i.e., s8(8b) to s8(32b))?
It seems to me we are trying to write <4 x s8> = s8(32b), ...., but maybe we want <4 x s8> = s32, ....
I've had a think about what having separate type and container sizes would actually mean. For brevity, let’s call values which have distinct type and container sizes “partial values”. A partial value s8:s32 means that the upper 24 bits of this 32 bit register is always undef. For some operations, it should always be possible to ignore the type size and just use the container size for selection. E.g. non-flag setting G_ADD could select a 32 bit add and satisfy the semantics of an s8 add. But for other operations like shifts, or count-leading-zeros, we can't use a wider operation type, so the entire chain of partial value operations would be restricted to selecting the narrower type, rather than the container. And at that point, the container type may as well not be there. There may be a way around this issue I haven't seen though.
Anyhow, to be concrete, I am fine with the implicit truncate, we know this is not too bad based on SDISel experience, I want to make sure we consider all our options here.
Sure. Given that we already have this implicit truncating behavior on account of G_MERGE being an omnipotent operation, should we allow the implicit truncation with G_BUILD_VECTOR for now? If we have problems with it later we can revisit the idea of adding a separate opcode/something else.
Oct 30 2018
LGTM with the nits addressed.
Oct 29 2018
Updated AArch64 legalization. I've removed the unsupported case since we're actually making it valid.
Updated patch to allow G_BUILD_VECTOR to have scalar operands not identical to the result vector element types. This allows implicit truncation.
Oct 26 2018
Oct 25 2018
Committed in r345265.
Oct 24 2018
+ @huntergr On a tangential note, G_BUILD_VECTOR for expressing broadcasts of scalars won't work for SVE and other variable vector width targets. We do however still leave the door open here for a new G_BROADCAST opcode in future.
Oct 23 2018
Oct 15 2018
Thanks for working on this. Apart from Daniel's suggestion, I have a few more comments but haven't done an in depth analysis of the memory costs/algorithmic properties.
Oct 11 2018
Oct 9 2018
Oct 1 2018
Sep 20 2018
@aditya_nandakumar I've added a target hook that defaults to enabling this.
This seems like something that most targets would want done by default. I wonder if we should have a standard set of legalizer actions that targets can use. LGTM anyway.
Sep 17 2018
This isn't really ideal because of how trivial it is currently. The issue is that if we added the capability to InstSimplify (which is where it would fit most naturally), we'd still end up with the problem of running the whole of InstSimplify in the GlobalISel pipeline. It's reasonable for now I think so I'll commit with the changes requested.
Looks ok, if a bit cumbersome with the additional state (do we need to hold references to it?). I'd like to see how this state is used with the CSE builder first though.
Sep 13 2018
Ok, I'm surprised that would be a realistic scenario but I'll make the change. Thanks.
Sep 12 2018
You're right, that's a simplified expression.
Sep 11 2018
Sep 8 2018
Sep 5 2018
Sep 4 2018
Ok, to me it seems like instructions for which fast math flags can't apply shouldn't be classed as FPMathOperators. insertelement/extractelement are just moving raw bits. If we disallowed those instruction types in the FPMathOperator classof() then this problem should go away, and we don't incorrectly relax FP semantics like in the case I pointed out.
Aug 31 2018
I’m not seeing how FP instructions can not carry flags. The IR semantics are defined to be strict FP unless explicitly relaxed.
Hit submit too early:
So with the above code B will not have the flag cleared? If that's the case it doesn't seem right to me.