[flang-commits] [PATCH] D106820: [flang] Fix runtime ICE with maxloc and scalar result

[Mark LeAir via Phabricator via flang-commits]

mleair added inline comments.


================
Comment at: flang/runtime/reduction-templates.h:209
+    result.GetLowerBounds(at);
   INTERNAL_CHECK(at[0] == 1);
   using CppType = CppTypeFor<CAT, KIND>;
----------------
klausler wrote:
> mleair wrote:
> > mleair wrote:
> > > klausler wrote:
> > > > `INTERNAL_CHECK(result.rank() == 0 || at[0] == 1);`
> > > Hi Peter,
> > > 
> > > I thought about that change, but I  think "at" is still referenced after this check. For example:
> > > 
> > >  // No MASK= or scalar MASK=.TRUE.
> > > for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {
> > >     accumulator.Reinitialize();
> > >     ReduceDimToScalar<CppType, ACCUMULATOR>(
> > >         x, dim - 1, at, result.Element<CppType>(at), accumulator);
> > >   }
> > > 
> > > It looks like ReduceDimToScalar() still references "at". If so, then "at" should be initialized, correct? I believe this is where the UMR comes from. When at[0] is initialized to 1, the UMR goes away.
> > > 
> > To clarify, I think there might be a second UMR with with ReduceDimToScalar(); not just with the INTERNAL_CHECK macro.
> I doubt it.  When rank == 0, the subscript values should not be used by Descriptor::SubscriptsToByteOffset() (and therefore not by Descriptor::Element) or by ReduceDimToScalar.  How do you know that ReduceDimToScalar() is referencing uninitialized memory?
Sorry. I was mistaken. I thought I still saw a valgrind bug when I tried the rank guard with the INTERNAL_CHECK macro, But when I tried it just now, it does not occur. I'll check the other cases too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106820/new/

https://reviews.llvm.org/D106820