[PATCH] D100015: [debug-info] make fortran CHARACTER(1) type as valid unsigned type

[LiuChen via Phabricator via llvm-commits]

LiuChen3 added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:180
+  if (isa<DIStringType>(Ty)) {
+    assert((Ty->getSizeInBits()) == 8 && "Not a valid unsigned type!");
+    return true;
----------------
LiuChen3 wrote:
> shchenz wrote:
> > LiuChen3 wrote:
> > > aprantl wrote:
> > > > LiuChen3 wrote:
> > > > > shchenz wrote:
> > > > > > LiuChen3 wrote:
> > > > > > > Hi, @shchenz . This assertion make our testcase fail on X86 target. I am not familiar with this part so I have a question here: why the type here must be unsigned? Should this be something like "return Ty->getSizeInBits() == 8; " ?
> > > > > > > Thanks.
> > > > > > I added this assertion only for `i8` is because SROA will convert Fortran `CHARACTER(1)` type (`DIStringType`) to `i8`. Do you also get some ` DIStringType` variables being converted to a scalar type, like `i8`, `i16`, `i32`? If so, I think you need to first check if the above conversion is valid or not. if it is valid, then you can change the assertion here.
> > > > > Our test is too big and I didn't find the root cause now.
> > > > > I am just confused about the assertion here. If the DIStringType must be i8, I think you should insert assertion after the convert(Maybe in SROA), instead of inserting assertion here. Here should just return false if the type isn't i8.
> > > > We have many tools like delta (https://web.archive.org/web/20200701152100/http://delta.tigris.org/) bugpoint, and llvm-reduce available which can automate the process of reducing a testcase.
> > > The spec is too big and this fail happened on link time.  With '-g' make it slower.
> > > I still wonder why you think the DIStringType here must be unsigned char? No i16, i32 or some types else?
> > [1xi8] should be ok to convert to i8 as the data layout would be no change. Imaging `char` -> `int` in `.c`. But [2xi8](`CHARACTER(2)`) should be non-ok to convert to i16 as the data layout is not the same, we can not convert `char[2]` to `int`?
> You should insert the assertion in SROA if you thought the [1xi8] must do this convert.  No assumptions should be made about type of DIStringType here.
> Suppose this function is like this: bool ischar(Type ty)
> Here 'ty' can be 'char', 'int', 'long long' or any types. Just return false if the type isn't 'char' instead of crashing.
I understand what you mean a little bit. The DIStringType can only be i8 here. But I still feel that the assertion here does not match the meaning of the function. I will continue to try to find out if some of our passes are wrong as you said. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100015