[PATCH] [x86] add a reassociation optimization to increase ILP via the MachineCombiner pass

[Gerolf Hoflehner]

Thanks for iterating on this! LGTM!

-Gerolf
 
> On Jun 10, 2015, at 9:25 AM, Sanjay Patel <spatel at rotateright.com> wrote:
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6234
> @@ +6233,3 @@
> +/// reassociation candidate, Commuted will be set to true.
> +static MachineInstr *isReassocCandidate(const MachineInstr &Inst,
> +                                        unsigned AssocOpcode,
> ----------------
> Gerolf wrote:
>> I'm not happy about the check* parameters that control the pattern match, but I guess it is better to revisit that at a later point when you will support more reassociation pattern. Although I still suggest removing checkPrevOpcode even for this commit.
> Sounds good - I removed the checkPrevOpcode param.
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6286
> @@ +6285,3 @@
> +/// need to be commuted.
> +static MachineCombinerPattern::MC_PATTERN getPattern(bool CommutePrev,
> +                                                     bool CommuteRoot) {
> ----------------
> Gerolf wrote:
>> If you like, I think this could be implemented this by by a 2x2 matrix and avoid the conditionals.
> I know we're already relying on the enum values as indexes below, but I'd prefer to keep this logic clearer for now instead of encoding.
Sure. I was not convinced about the 2x2 thing either. This approach might become more interesting when more pattern get supported.
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6293
> @@ +6292,3 @@
> +  }
> +  // !CommutePrev
> +  if (CommuteRoot)
> ----------------
> Gerolf wrote:
>> else? 
> I was originally going with this:
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
> 
> ...but I prefer the symmetry of an 'else' here. Fixed.
I know this was ‘edgy’. But the purpose of the coding standard is to improve readability and in this case I would argue the ‘else’ does that.
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6313
> @@ +6312,3 @@
> +  bool CommuteRoot;
> +  if (MachineInstr *Prev = isReassocCandidate(Root, AssocOpcode, true, true,
> +                                              CommuteRoot)) {
> ----------------
> Gerolf wrote:
>> For this commit that parameter could be eliminated. I understand your intention, but it confusing for anyone that comes across the code the first time. So I would add it in a later commit. That will do away with the TODO also.
> Param removed, however, I think that just pushes the TODO into isReassocCandidate() until we support the general case.
> 
> I do intend to continue working on this, so hopefully, the TODOs are quite temporary. :)
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6334
> @@ +6333,3 @@
> +/// Attempt the following reassociation to reduce critical path length:
> +///   A = (Anything)
> +///   B = A op X (Prev)
> ----------------
> Gerolf wrote:
>> Right now the code supports A = ?? op ??. I would make that clear in the comment and change it later in the follow up patch that generalizes this one. Same for the equivalent comments above and below.
> I made the comments line up with the more general logic that is actually implemented - so we don't even need the 'A' instruction line. The current limitation on the 3rd instruction is noted in the TODO comment in isReassocCandidate().
> 
> ================
> Comment at: lib/Target/X86/X86InstrInfo.cpp:6436
> @@ +6435,3 @@
> +
> +  reassociateOps(Root, Prev->getOperand(OpIdx[Pattern][0]),
> +                 Root.getOperand(OpIdx[Pattern][1]), Root.getOperand(0),
> ----------------
> Gerolf wrote:
>> I think you could make the array static and only pass Root and Prev to reassociateOps(). That would result in a smaller  signature.
> I may not have implemented this as you were envisioning: I moved the array and op selection into reassociateOps(). Now we have the smaller signature, but we need to pass the Pattern value as the selector. Let me know if you see a better way. Thanks!
> 
> http://reviews.llvm.org/D10321
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> 
>