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

[Andrew Trick]

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