[llvm-commits] [PATCH] Make dependency DAG construction to use AA

[Sergei Larin]

Andy, 

  Here is the latest patch version, addressing your comments. I still call
getUnderlyingObject() for reasons mentioned in comments.
It does look cleaner that way, so hopefully we are getting closer to being
done.

Sergei

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.


> -----Original Message-----
> From: Andrew Trick [mailto:atrick at apple.com]
> Sent: Thursday, May 10, 2012 8:55 PM
> To: Sergei Larin
> Cc: 'Hal Finkel'; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Make dependency DAG construction to
> use AA
> 
> On May 10, 2012, at 1:01 PM, Sergei Larin <slarin at codeaurora.org>
> wrote:
> > Andy, Hal,
> >
> >  Here is an updated patch. I hope I got all the comments. It does
> > include dep latency set now as well.
> > If I missed something - please let me know.
> > I also enabled the test for march=arm for now (they actually call
> that
> > code), but placed the test under CodeGen/Hexagon.
> > My intent is to change that to march=hexagon once Hexagon switches to
> > MI scheduling.
> >
> >  Thanks.
> >
> > Sergei
> >>
> >>> I'm not sure why you're doing this. What are the actual
> >>> preconditions that we need before calling AliasAnalysis?
> >>
> >>
> >> [Larin, Sergei] I was trying to be as unadventurous as possible
> here,
> >> and yet again err on the side of conservatism. Previously here was a
> >> complex set of checks for volatility, presence of just one memory
> >> objects (I convinced myself to start with the simplest case of
> >> hasOneMemOperand() to facilitate review and comprehension of the
> >> patch, and extend if needed later. Also my back end never produces
> >> multiple mem operands, so I do not yet feel comfortable to proceed
> >> without testing. I'll put a FIXME here). There were also more checks
> >> for analyzability of the memory operand.
> >> Anyway, once I compared all these checks to the functionality of
> >> getUnderlyingObjectForInstr(), it was 99% match, so I reduced code
> >> duplication by reusing existing logic.
> 
> +  // Using getUnderlyingObjectForInstr in its entirety might be a
> + slight  // overkill, but it guarantees consistency in approach to the
> + rest  // of DAG construction, and might prevent any future confusion
> + // if it is changed for some reason.
> +  if (!getUnderlyingObjectForInstr(MIa, MFI, MayAlias) ||
> +      !getUnderlyingObjectForInstr(MIb, MFI, MayAlias))
> +    return true;
> 
> The main thing getUnderlyingObjectForInstr() does is call
> getUnderlyingObject(). Calling that here doesn't make sense to me
> conceptually, and the comments in the code don't adequately explain why
> you're using the API. If I decide to refactor this later, I have no
> idea what functionality you depend on.
> 
> I agree we should be conservative, but not by arbitrarily limiting the
> problem size. Replace these calls with explicit checks for the
> conditions that might be problematic, and explain why we can't handle
> them. If you want to reuse code with getUnderlyingObjectForInstr, you
> can do that by factoring the code, not calling it directly.
> 
> +  MachineMemOperand *MMOa = *MIa->memoperands_begin();
> 
> Your concern is that you can't test the interesting case yet. So you
> should explicitly mark this unimplemented by adding a FIXME followed by
> a check for hasOneMemOperand() and llvm_unreachable. Then do *not*
> filter hasOneMemOperand above because we need to hit this error rather
> than silently ignoring it. Obviously it needs to be fixed before going
> default.
> 
> That's it. Everything else looks good.
> 
> -Andy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_aa_in_dag_v3.patch
Type: application/octet-stream
Size: 17635 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120511/6a872c0f/attachment.obj>