[PATCH] Alignment assumptions using invariants

Andrew Trick atrick at apple.com
Thu Jul 31 19:31:27 PDT 2014


Why does this pass run late? Is it because it's only intended to prepare for ISEL? What happens if AlignmentFromAssumptions runs early? Are there transformations that don't preserve load alignment info after you generate it? Unless there's a good reason, it should run before passes that require SCEV or after passes that preserve SCEV. There may have been a thread on this that I missed/forgot, but I think the motivation/justification should be commented somewhere, like the file header.

AlignmentFromAssumptions::runOnFunction is giant. I like it to see the full scope of a loop within a single page of code and like the lifetime of local variables to be obvious. The logic to deduce alignment from llvm.assume can be factored out, as well as the logic for processing each use on the worklist.

The NewAlignment for MemTransferInst logic is a little hard to follow. Can you just have something roughly like this:

  NewDestAlign = max(OldDestAlign, getNewAlignment(..., MI->getDest()))
  NewSrcAlign = max(OldSrcAlign, getNewAlignment(..., MI->getSource()))
  NewAlign = min(NewDestAlign, NewSrcAlign).

When you have a MemInstrinsic but know it's not MemTransfer, you set the alignment unconditionally. I think you should explicitly check for MemSetInst instead.

I think you are using SCEV primarily to determine whether two induction variables have a constant difference. e.g.

  const SCEV *DiffSCEV = SE->getMinusSCEV(PtrSCEV, AASCEV);

That's fine, but I would avoid overusing SCEV. For example, getNewAlignmentDiff() would be much more efficient operating on constants. Is there any value in using SCEV do perform arithmetic in that case. In general, only use SCEV if you need to reason about induction variables.

I didn't review isValidAssumeForContext yet, because I need to find the patch it is in.

================
Comment at: lib/Transforms/Scalar/AlignmentFromAssumptions.cpp:44
@@ +43,3 @@
+
+namespace {
+  struct AlignmentFromAssumptions : public FunctionPass {
----------------
I never indent within namespace. I don't see any value in it.

http://reviews.llvm.org/D181






More information about the llvm-commits mailing list