- User Since
- Aug 19 2013, 3:30 PM (230 w, 1 d)
I agree, we need the classes when printing single/few instructions. I also think this generally makes it harder to debug things which is contradictory to the purpose of -debug.
Fri, Jan 12
LGTM with some nits fixed
Tue, Jan 2
GICombinerHelper - this contains transformations that are common to all targets. Targets can pick and choose which transformations (at function/opcode granularity) each pass uses via configuring a GICombinerInfo.
Thu, Dec 21
Wed, Dec 20
Mon, Dec 18
Dec 11 2017
LGTM. We need to follow up with the refactor to eliminate clearImplicitMap() though. We ought to follow up with the simplification of PredicateListMatcher too. The templating isn’t very useful anymore now that predicates have a common base class
Dec 4 2017
Dec 1 2017
Nov 30 2017
Nov 29 2017
Yes, the canonicalization takes place in the target-independent part of the SelectionDAG code (SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1, SDValue N2, const SDNodeFlags Flags).
I guess one advantage that DAGISel has over GlobalISel is that the getNode helpers are used throughout instruction selection. If we wanted the same kind of behaviour for GlobalISel, we'd have to put this canonicalization in the MachineInstrBuilder, but that would affect the whole backend.
I think for now it's better to keep this in the IRTranslator since it's the least disruptive thing to do, and any subsequent passes (e.g. combiners and whatnot) can benefit from having a canonical form to work with most of the time. An alternative would be to move these things into their own canonicalization pass that we can then schedule whenever we see fit, but that might be a bit overkill since adding canonicalizations isn't really a priority at this point.
Nov 28 2017
I think GIM_CheckLiteralInt might have the same issue but I don't think there's any existing rules that would detect the problem. As far as I know X86 is the only user and only uses 0 and 1. We should probably change it to match GIM_CheckConstantInt if only for consistency. Do you agree?
Nov 27 2017
Nov 17 2017
Rename G_ATOMIC_CMPXCHG to G_ATOMIC_CMPXCHG_WITH_SUCCESS. There is a gap between
the LLVM-IR cmpxchg instruction and the SDNode that is usable in SelectionDAG
patterns. IRTranslate into a LLVM-IR-like representation. We'll then bridge the
gap in the legalizer.
Nov 16 2017
Include the CHECK lines in the patch.
Nov 15 2017
I see why ARM is different from AArch64. ARM only sees one pattern being emitted because TableGen has some code that prevents commutativity being considered when one of the children of a commutative operator is either the imm operator or a plain integer (see OnlyOnRHSOfCommutative() which is used in canPatternMatch()). Meanwhile, AArch64's case is using a ComplexPattern so both cases get emitted.
Nov 13 2017
Nov 10 2017
Nov 9 2017
Nov 8 2017
Fix covered() -> setCovered() change that was somehow missed, and the other nits.
Nov 7 2017
CodeGenCoverage::covered() -> setCovered()
Use SmartMutex and SmartScopedLock
Add missing docstring
Nov 2 2017
LGTM. This must have been missed because they're genuinely operands. A lot of the predicates (particularly those on imm and fpimm) are on operators even though they look like operands.
Nov 1 2017
Oct 31 2017
Oct 24 2017
There are a lot of changes in this patch so I haven't gone through the detail but this seems to be heading in the general direction I had in mind for when we got to optimizing the rules.
Tracing the legalizer is still something we need to improve on but this patch isn't suitable given that we aren't restricting the insertion point.
Oct 23 2017
Reduce patch to just the bit about applying this to GPR32/GPR64/GPR32all/
GPR64all. I found a way to constrain the original to just the relevant stores.
Oct 20 2017
There's two patches in one here and I'm going to try and extract the stores portion of the patch and commit that separately if I can find a reasonable way to do so without causing regressions. The main purpose of this review is to determine whether we agree it's safe to allow the new GIZeroRegister on GPR32/GPR64/GPR32all/GPR64all. I think it's probably ok, but I don't feel I have sufficient knowledge of AArch64 to make such a sweeping change without someone more familiar with it giving it the go ahead.