[CIR][Codegen] Fixes global variables that point to another globals

[gitoleg]

Testing CIR with csmith is in progress! And this PR is not a small one, so I need to apologize. But looks like there is a problem that may affect the run time.

Problem

Consider the next code:

#include  <stdio.h>
typedef struct {    
   int f0 : 24;      
   int f1;
   int f2;
} S;

static S g1 = {2799, 9, 123};
static int *g2 = &g1.f2;

int main() {
 printf("check: %d\n",*g2);  
 return 0;
}

This program dumps anything but not 123.
So basically we don't support global variables that refer to another globals in the case of aggregate types.
This PR fixes global views for two cases: structs and array of structs (see the tests).

There is an issue with unions too, but I think we will discuss it later, once we will agree on the approach

Some details

For the example above, g1 variable is created in two steps:

1. The variable is created with the S type. In our case it's !cir.struct<struct "S" {!cir.array<!cir.int<u, 8> x 3>, !cir.int<s, 32>, !cir.int<s, 32>}(which differs from the OG btw, and may be I need to take a look at the bit fields again - but it's not a problem right now)
2. For the left side we create anon structure of type !cir.struct<struct {!cir.int<u, 8>, !cir.int<u, 8>, !cir.int<u, 8>, !cir.int<u, 8>, !cir.int<s, 32>, !cir.int<s, 32>}> which is the same as in OG and then g1 is replaced with the new type.

Basically the same happens in the OG. But then the replaceAllUsesWith solves all the possible problems.
In our case we can not do it this easy. The g2 is:

cir.global @g2 = #cir.global_view<@g1, [2 : i32]> : !cir.ptr<!s32i> 

The problem is in the indexes! After g1 is replaced, the index in g2's GlobalViewAttrstill points to the old type!!!
So we have to create a new GlobalViewAttr with new indexes!

Solution

My solution is based on the computeGlobalViewIndicesFromFlatOffset function from CIRGenBuilder that is basically a mapping from indexes to offset. I compute an offset and then map it back to the indexes with for the new type.

May be there is a better solution though, have some ideas?

Implementation details

Most of the changes are in the CIRGenModule where we do the replacement.
Also, there are some changes in CIRGenBuilder - I moved computeGlobalViewIndicesFromFlatOffset to cpp with no changes and added the opposite function computeOffsetFromGlobalViewIndices.

One more fix is more important - is about GlobalViewAttr indexes generation for vtable. I suggest we don't set the first index to zero in CIR and add it in the lowering - in this case we can do it uniformly as

if (isa<mlir::LLVM::LLVMArrayType, mlir::LLVM::LLVMStructType>(sourceType))
      indices.push_back(0);    

The previous approach was not completely correct - we have to add the leading zero index for the anon structures as well:

if (stTy.isIdentified())
        indices.push_back(0); // Wrong

Thus, there are some changes in tests as well. It's not an unrelated issue - we have to fix it - either now or as a separated PR that should come before this one.

Also, I added tests with the covered cases with the original LLVM IR, just for comparison - so there are some divergences as well.

Finally - this code was already tested with csmith and I don't have any problems so far. Once we'll fix unions I will have more information.

[gitoleg]

Actually, there is another approach for the problem. Instead of rewriting indices we could store an offset in the global view attribute here. The OG does the same: creates a pointer to i8 and craeted gep with offset from it. That's where the IR difference comes from. So in our case it would look like

mlir::ArrayAttr getOffset(mlir::Type Ty) {
  auto Offset = Value.getLValueOffset().getQuantity();
  if (Offset == 0)
    return {};
  
  llvm::SmallVector<mlir::Attribute, 3> Indices;
  auto Attr = CGM.getBuilder().getI32IntegerAttr(Offset);
  Indices.push_back(Attr);
  return CGM.getBuilder().getArrayAttr(Indices);
}

ConstantLValue applyOffset(ConstantLValue &C) {

  // Handle attribute constant LValues.
  if (auto Attr = mlir::dyn_cast<mlir::Attribute>(C.Value)) {
    if (auto GV = mlir::dyn_cast<cir::GlobalViewAttr>(Attr)) {
      assert(!GV.getIndices() && "Global view is already indexed");
      auto baseTy = mlir::cast<cir::PointerType>(GV.getType()).getPointee();
      auto destTy = CGM.getTypes().convertTypeForMem(DestType);        
      auto offset = getOffset(baseTy);
      
      if (offset) {
        auto typ = CGM.getBuilder().getUInt8Ty();
        destTy = CGM.getBuilder().getPointerTo(typ);
      }
      
      return GlobalViewAttr::get(destTy, GV.getSymbol(), offset);      
    }
    llvm_unreachable("Unsupported attribute type to offset");
  }

  // TODO(cir): use ptr_stride, or something...
  llvm_unreachable("NYI");
}

From the other point of view this approach can be too low level for CIR. But we can go this way if you prefer - in this case CIRGenModule may not be touched and LLVM IR generated will be closer to the OG one.

[bcardosolopes]

Seems like recent updates are triggering failures

[gitoleg]

updated!

[bcardosolopes]

From the other point of view this approach can be too low level for CIR. But we can go this way if you prefer - in this case CIRGenModule may not be touched and LLVM IR generated will be closer to the OG one.

This sounds appealing, can you paste the CIR/LLVM output for the two different approaches so I can give a more informed take?

[gitoleg]

Say we have the next structure type and global vars:

typedef struct {
    char a1;
    char a2;
    char a3;
} S;

S s[10];
char *elt = &(s[2].a3);

The OG LLVM IR:

@s = dso_local global [10 x %struct.S] zeroinitializer, align 16
@elt = dso_local global ptr getelementptr (i8, ptr @s, i64 8), align 8

With the approach in question

The LLVM IR with CIR and this approach applied:

@s = global [10 x %struct.S] zeroinitializer, align 16
@elt = global ptr getelementptr inbounds (i8, ptr @s, i32 8), align 8

There are some differences, but gep instructions almost the same.
But since we use offsets, CIR looks like the following:

cir.global external dsolocal @s = #cir.zero : !cir.array<!ty_S x 10> {alignment = 16 : i64}
cir.global external dsolocal @elt = #cir.global_view<@s, [8 : i32]> : !cir.ptr<!u8i> {alignment = 8 : i64} 

In this case the lowering will require special handling for this GlobalViewAttr with offset - but we will fix it. And we get LLVM IR that looks almost the same. But the cost is cir readability - it's hard to say what the exact array element and field is accessed.

In the current CIR's state

The LLVM IR (with CIR):

@s = global [10 x %struct.S] zeroinitializer, align 16
@elt = global ptr getelementptr inbounds ([10 x %struct.S], ptr @s, i32 0, i32 2, i32 2), align 8

CIR itself:

cir.global external dsolocal @s = #cir.zero : !cir.array<!ty_S x 10> {alignment = 16 : i64}
cir.global external dsolocal @elt = #cir.global_view<@s, [2 : i32, 2 : i32]> : !cir.ptr<!s8i> {alignment = 8 : i64}

The point is - in this case it's more clear for the reader what the element is accessed - the array element with index 2 and then a field with index 2. And the lowering may be improved later in order to produce more OG-like IR.

[bcardosolopes]

For the readability sake, I guess that a lot depends on how you read it, in the (a) first version:

cir.global external dsolocal @elt = #cir.global_view<@s, [8 : i32]> : !cir.ptr<!u8i> {alignment = 8 : i64}

We are saying that we are first looking at the "final" type !cir.ptr<!u8i>, and then applying a 8 position change. Whereas:

cir.global external dsolocal @elt = #cir.global_view<@s, [2 : i32, 2 : i32]> : !cir.ptr<!s8i> {alignment = 8 : i64}

In this one (b) we'd have to look at the original @s type for those indicies to make sense. For the future: perhaps we should change the syntax to @s in [2 : i32, 2 : i32] or [2 : i32, 2 : i32] in @s to make it more obvious.

What I'm really trying to make sure with as part of this discussion is that we avoid a mix of these semantics, the indices should always talk about the source type or always about the resulting type.

My take is that we should stay with (b) if you have found a way to make it work with the bug you are fixing, otherwise we could go for (a), but we need to change docs and make sure it's consistent across the board.

[gitoleg]

Looks like I meet the case (thanks to csmith) when we still need (a), i.e. use offsets. The problem is that replaceGlobal is not called for some reason, i.e. don't have a chance to recompute the indexes. And the same is in OG - there is no replacement as far I can see, so most likely it's not a bug somewhere in our code, and the OG works because the offsets are used.

Let me spend a couple more hours - may be I'll find a solution to keep the existent approach.
It would be sad if all I did before won't be useful though (

[bcardosolopes]

@gitoleg thanks for going the extra length, I appreciate it.

It would be sad if all I did before won't be useful though

Indeed, but on the bright side it was necessary in order to pinpoint the investigation, hopefully we'll find a middle ground!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[gitoleg]

@bcardosolopes

I fixed the issue that makes me sad the last time we talked here and covered several more cases as well - but it less important.

Basically the problem was that all this globals replacements was not used (the same in OG). The solution is to create a global view with the mlir type of a global var we want to take a view on but now with its AST type (changes in getAddrOfGlobalVar).

Now I think the PR is ready for merge - once you have no objection of course.
I tested with csmith - and so far there is no new problems in the codegen . Anyway, I continue this work - will see what else can be done here