[llvm-commits] [PATCH] Multidimensional Array Index Delinearization Analysis

[Hal Finkel]

----- Original Message -----
> Thanks Hal for the review! It much improved the code:

Great!

> 
> Hal Finkel wrote:
> > +  // Removes from Start all multiples of Step and Remainder. The
> > division is
> > +  // guaranteed to succeed as Step and Remainder have been pattern
> > matched with
> > +  // SCEVGcd::findGcd().
> > 
> > It seems like there should be an assertion somewhere in here to
> > very this.
> 
> After I added this assert, I had to rework all the GCD and division
> code for
> them to work in sync.  As a result, the changes to the data
> dependence testsuite
> look much better.
> 
> > +namespace {
> > +struct SCEVGcd : public SCEVVisitor<SCEVGcd, const SCEV *> {
> > 
> > SCEVGcd looks ugly to be, I think that SCEVGCD would be better
> > (actually, I'd replace Gcd with GCD everywhere).
> 
> fixed.
> 
> > 
> > +    if (const SCEVConstant *Cst = dyn_cast<SCEVConstant>(Gcd)) {
> > +      const SCEV *Res =
> > SE.getConstant(APIntOps::GreatestCommonDivisor(
> > +          Cst->getValue()->getValue(),
> > Constant->getValue()->getValue()));
> > +      if (Res != One)
> > +        return Res;
> > +    }
> > 
> > Are you sure that Cst and the constant will always have the same
> > number of bits? (what if we've walked through a sign extension,
> > truncation, etc.)
> > 
> > I have the same comment about visitConstant in SCEVDivision.
> 
> I have not understood what this comment implies and whether there
> should be an
> extra assert or change in the code.  Hal, could you please explain a
> bit more in
> which situation you see this a problem?
> 

I think that there should be a change in the code. If we have an input sub-expression that looks like:

 zext (x * 4) from i32 to i64

but by the time that visitConstant is called on the (i32 4), I could have GCD = (i64 8), for example. Then then we'd try calling APIntOps::GreatestCommonDivisor (or some other APInt operation) on (i32 4) and (i64 8), which will assert. However we do it, both constants need to be of the same bit length for the operation to work.

 -Hal

> 
> > +attributes #0 = { nounwind uwtable "less-precise-fpmad"="false"
> > "no-frame-pointer-elim"="false" "no-infs-fp-math"="false"
> > "no-nans-fp-math"="false" "stack-protector-buffer-size"="8"
> > "unsafe-fp-math"="false" "use-soft-float"="false" }
> > +
> > +!0 = metadata !{metadata !"int", metadata !1}
> > +!1 = metadata !{metadata !"omnipotent char", metadata !2}
> > +!2 = metadata !{metadata !"Simple C/C++ TBAA"}
> > 
> > Remove the unneeded attributes and TBAA metadata from the tests.
> 
> done.
> 
> Attached the revised patch.
> 
> Thanks,
> Sebastian
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory