Propagate the volatile qualifier of exp to store /load operations .

[umesh.kalappa0]

Summary

This changes to address the PR : 55207

We update the volatility on the LValue by looking at the LHS cast operation qualifier and propagate the RValue volatile-ness from the CGF data structure .

[aaron.ballman]

Adding some more codegen reviewers; the changes should come with a re

clang/lib/CodeGen/CGExprScalar.cpp

2236–2240

clang/lib/CodeGen/CodeGenFunction.h

548–549

I'm not quite sure how to repair the second sentence; were you going for "doesn't have to have the volatile qualifier"?

[efriedma]

Using a function-wide "isVolatileCastStmt" will almost certainly break in more complex cases (consider, for example, the case where the address of a load itself contains loads).

[rjmccall]

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

[rjmccall]

Slight correction: I guess a NoOp cast can *remove* qualifiers, too. We should just unconditionally use the volatile qualifier from the result type of the cast instead of only *adding* it.

[umesh.kalappa0]

In D157890#4585753, @efriedma wrote:

Using a function-wide "isVolatileCastStmt" will almost certainly break in more complex cases (consider, for example, the case where the address of a load itself contains loads).

@efriedma ,thank you for the pointers and we thought of propagating this info from VisitCast ,but that needs various api change and how about the reset "isVolatileCastStmt " to false after use in "VisitCast" ,that resolve this complex address load issue .

or any other suggestions you have in mind ?

[umesh.kalappa0]

In D157890#4585937, @rjmccall wrote:

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

@rjmccall ,we need "isVolatileCastStmt" for the RHS that has the cast like stated in the testcase and "EmitCastLValue" not handle the RValue .

[umesh.kalappa0]

In D157890#4585953, @rjmccall wrote:

Slight correction: I guess a NoOp cast can *remove* qualifiers, too. We should just unconditionally use the volatile qualifier from the result type of the cast instead of only *adding* it.

@rjmccall ,thank you and you right here but adding unconditionally can assert the case where either cast doesn't have the volatile qualifier nor result type doesn't have the same and if we remove the volatile-ness from LValue ,then it doesn't make sense to add right ?

[rjmccall]

In D157890#4587414, @umesh.kalappa0 wrote:

In D157890#4585937, @rjmccall wrote:

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

@rjmccall ,we need "isVolatileCastStmt" for the RHS that has the cast like stated in the testcase and "EmitCastLValue" not handle the RValue .

Oh, I think I understand the underlying issue with the RHS here, and it was something I was overlooking it before. Clang's r-value emitters aggressively recurse through LValueToRValue conversions, essentially sinking the load into the emission of the wrapped l-value expression. This is good to do because there are some common situations (especially in C++) where expressions are formally l-values but we can emit them more efficiently if we know there's going to be an immediate load. When we're sinking loads that way, it's usually okay to also recurse through a NoOp conversion. But we can't do that if there's a change in qualification that'll affect how the load is emitted, which mostly means a change in volatile.

Using a "global" flag on the CGF object is still the wrong way to solve this. The r-value emitters need to detect that a cast materially changes qualification (I think this just means volatile) and, if so, emit the expression as an l-value and then load from it rather than aggressively recursing through the cast.

You'll need to do this same logic in all of the r-value emitters: scalar, complex, and (probably) aggregate. You should be able to write a common helper function that says whether a CastExpr has a material qualification difference.

[umesh.kalappa0]

In D157890#4589020, @rjmccall wrote:

In D157890#4587414, @umesh.kalappa0 wrote:

In D157890#4585937, @rjmccall wrote:

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

@rjmccall ,we need "isVolatileCastStmt" for the RHS that has the cast like stated in the testcase and "EmitCastLValue" not handle the RValue .

Oh, I think I understand the underlying issue with the RHS here, and it was something I was overlooking it before. Clang's r-value emitters aggressively recurse through LValueToRValue conversions, essentially sinking the load into the emission of the wrapped l-value expression. This is good to do because there are some common situations (especially in C++) where expressions are formally l-values but we can emit them more efficiently if we know there's going to be an immediate load. When we're sinking loads that way, it's usually okay to also recurse through a NoOp conversion. But we can't do that if there's a change in qualification that'll affect how the load is emitted, which mostly means a change in volatile.

Using a "global" flag on the CGF object is still the wrong way to solve this. The r-value emitters need to detect that a cast materially changes qualification (I think this just means volatile) and, if so, emit the expression as an l-value and then load from it rather than aggressively recursing through the cast.

You'll need to do this same logic in all of the r-value emitters: scalar, complex, and (probably) aggregate. You should be able to write a common helper function that says whether a CastExpr has a material qualification difference.

Agree global flag is not the right way and refactor the changes for scalar/aggregate and complex and

whether a CastExpr has a material qualification difference.

tried to differentiate by QualType of declnode and expression with no success ,hence we go-ahead if the exp has volatile then we choose the other path .

[rjmccall]

In D157890#4591557, @umesh.kalappa0 wrote:

In D157890#4589020, @rjmccall wrote:

In D157890#4587414, @umesh.kalappa0 wrote:

In D157890#4585937, @rjmccall wrote:

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

@rjmccall ,we need "isVolatileCastStmt" for the RHS that has the cast like stated in the testcase and "EmitCastLValue" not handle the RValue .

Oh, I think I understand the underlying issue with the RHS here, and it was something I was overlooking it before. Clang's r-value emitters aggressively recurse through LValueToRValue conversions, essentially sinking the load into the emission of the wrapped l-value expression. This is good to do because there are some common situations (especially in C++) where expressions are formally l-values but we can emit them more efficiently if we know there's going to be an immediate load. When we're sinking loads that way, it's usually okay to also recurse through a NoOp conversion. But we can't do that if there's a change in qualification that'll affect how the load is emitted, which mostly means a change in volatile.

Using a "global" flag on the CGF object is still the wrong way to solve this. The r-value emitters need to detect that a cast materially changes qualification (I think this just means volatile) and, if so, emit the expression as an l-value and then load from it rather than aggressively recursing through the cast.

You'll need to do this same logic in all of the r-value emitters: scalar, complex, and (probably) aggregate. You should be able to write a common helper function that says whether a CastExpr has a material qualification difference.

Agree global flag is not the right way and refactor the changes for scalar/aggregate and complex and

whether a CastExpr has a material qualification difference.

tried to differentiate by QualType of declnode and expression with no success ,hence we go-ahead if the exp has volatile then we choose the other path .

CE->isGLValue() && CE->getType().isVolatileQualified() != CE->getSubExpr()->getType().isVolatileQualified()

[umesh.kalappa0]

In D157890#4592428, @rjmccall wrote:

In D157890#4591557, @umesh.kalappa0 wrote:

In D157890#4589020, @rjmccall wrote:

In D157890#4587414, @umesh.kalappa0 wrote:

In D157890#4585937, @rjmccall wrote:

The fix to EmitCastLValue looks correct and sufficient. All of this business with isVolatileCastStmt should be removed.

@rjmccall ,we need "isVolatileCastStmt" for the RHS that has the cast like stated in the testcase and "EmitCastLValue" not handle the RValue .

Oh, I think I understand the underlying issue with the RHS here, and it was something I was overlooking it before. Clang's r-value emitters aggressively recurse through LValueToRValue conversions, essentially sinking the load into the emission of the wrapped l-value expression. This is good to do because there are some common situations (especially in C++) where expressions are formally l-values but we can emit them more efficiently if we know there's going to be an immediate load. When we're sinking loads that way, it's usually okay to also recurse through a NoOp conversion. But we can't do that if there's a change in qualification that'll affect how the load is emitted, which mostly means a change in volatile.

Using a "global" flag on the CGF object is still the wrong way to solve this. The r-value emitters need to detect that a cast materially changes qualification (I think this just means volatile) and, if so, emit the expression as an l-value and then load from it rather than aggressively recursing through the cast.

You'll need to do this same logic in all of the r-value emitters: scalar, complex, and (probably) aggregate. You should be able to write a common helper function that says whether a CastExpr has a material qualification difference.

Agree global flag is not the right way and refactor the changes for scalar/aggregate and complex and

whether a CastExpr has a material qualification difference.

tried to differentiate by QualType of declnode and expression with no success ,hence we go-ahead if the exp has volatile then we choose the other path .

CE->isGLValue() && CE->getType().isVolatileQualified() != CE->getSubExpr()->getType().isVolatileQualified()

Thank you @rjmccall for the pointers and same doesn't work with the case like

static_cast<volatile _Complex float &>(cf) = static_cast<volatile _Complex float&>(cf) + 1;

CXXStaticCastExpr 0x55556c405410 'volatile _Complex float' lvalue static_cast<volatile _Complex float &> <NoOp>
`-ImplicitCastExpr 0x55556c4053f8 'volatile _Complex float' lvalue <NoOp> part_of_explicit_cast
  `-DeclRefExpr 0x55556c4053c8 '_Complex float' lvalue Var 0x55556c404fb8 'cf' '_Complex float'

[umesh.kalappa0]

@rjmccall ,is that changes looks good to land on master or do you have any other comments ?

[rjmccall]

clang/include/clang/AST/Expr.h

3557 ↗

(On Diff #551455)

This is not a good name for this. Maybe changesVolatileQualification?

Also, please move this down after all the methods that are just getting and setting stored properties of the CastExpr — right before getTargetFieldForToUnionCast seems like the right place.

We should probably consider giving qualification conversions their own CastKind instead of folding them into NoOp. (You don't need to do that in this patch, I'm just saying that this seems like a poor choice of representation.)

clang/lib/CodeGen/CGExpr.cpp

4810

As discussed, you need to set the qualifiers to the values in E->getType(), not merge them. If someone casts away volatile, we should emit a non-volatile access. (And you should test this.)

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3557 ↗

(On Diff #551455)

We should probably consider giving qualification conversions their own CastKind instead of folding them into NoOp. (You don't need to do that in this patch, I'm just saying that this seems like a poor choice of representation.)

@rjmccall agree on this

[rjmccall]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

Please don't use this-> when you don't have to and it isn't saying anything important (like contrasting with some other object of the same type).

clang/lib/CodeGen/CGExpr.cpp

4810

if (E->changesVolatileQualification())
  LV.setVolatile(E->getType().isVolatileQualified());

and please add a test case to check that this is emitted with a non-volatile load:

volatile int vx;
int x = const_cast<int&>(vx);

clang/lib/CodeGen/CGExprScalar.cpp

2241

Consistent indentation, please.

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

Agree

clang/lib/CodeGen/CGExpr.cpp

4810

Thank you @rjmccall for the suggestion and was wondering the exist check is taking care of volatile cast away and where it can break ?

clang/test/CodeGen/volatile.cpp

41

@rjmccall ,this line check should take care of non-volatile case.

[umesh.kalappa0]

clang/lib/CodeGen/CGExpr.cpp

4810

@rjmccall ,i think the reason we used addVolatile() on LV was that the setVolatile() mem or any other volatile setter member functions doesn't exit on the LV or we are missing something ? .

[rjmccall]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

Just getSubExpr(), not getSubExprAsWritten().

clang/lib/CodeGen/CGExpr.cpp

4810

Ah, sorry, I was looking at the wrong type.

Looks like you can just do LV.getQuals() = E->getType().getQualifiers().

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

@rjmccall ,the getSubExpr() doesn't work for the case like

CXXStaticCastExpr 0x55556c4e7330 'volatile _Complex float' lvalue static_cast<volatile _Complex float &> <NoOp>
`-ImplicitCastExpr 0x55556c4e7318 'volatile _Complex float' lvalue <NoOp> part_of_explicit_cast

`-DeclRefExpr 0x55556c4e72e8 '_Complex float' lvalue Var 0x55556c4e6ed8 'cf' '_Complex float'

where getSubExpr() ="ImplicitCastExpr 'volatile _Complex float' lvalue <NoOp> part_of_explicit_cast"
and isVolatileQualified() is true .

[rjmccall]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

That's fine. You'll recurse one step and then catch when looking at the ICE.

[rjmccall]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

No, I mean that I don't think you need to do anything to handle this explicitly, because I would expect that IRGen will just recurse one step and then see the volatile change on the inner CE. That is:

• First, ScalarExprEmitter will look at the outermost cast and see that it's a NoOp that doesn't change volatile qualification, so it'll recurse to the sub-expression
• Next, ScalarExprEmitter will look at the middle cast and see that it's a NoOp that does change volatile qualification, so it'll do an EmitLoadOfLValue and do a volatile load.

Is that not what you're seeing?

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3622 ↗

(On Diff #552027)

Is that not what you're seeing?

@rjmccall ,thank you for inputs and yes that work for ScalarExprEmitter and not for ComplexExpr and we made changes accordingly now ,please let us know your thoughts on the same .

[rjmccall]

clang/include/clang/AST/Expr.h

3619 ↗

(On Diff #553377)

Please make this a const method so that you don't need these unnecessary const_casts wherever you call it.

This comment is over-precise; please use something more like:

Return true if this conversion changes the volatile-ness of a gl-value.
Qualification conversions on gl-values currently use CK_NoOp, but
it's important to recognize volatile-changing conversions in clients like
code generation that normally eagerly peephole loads. Note that the
query is answering for this specific node; Sema may produce multiple
cast nodes for any particular conversion sequence.

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3619 ↗

(On Diff #553377)

@rjmccall ,updated with changes .

[rjmccall]

clang/test/CodeGen/volatile.cpp

41

Please test the case where a cast removes a volatile qualifier. You're not testing that here because the underlying l-value is not volatile in the first place — I could tell this even without looking at the declaration of vol because a static_cast is not allowed to remove qualifiers.

[umesh.kalappa0]

clang/test/CodeGen/volatile.cpp

41

@rjmccall ,updated accordingly ,thanks

[rjmccall]

LGTM, thanks

clang/lib/CodeGen/CGExpr.cpp

if (Constant.isReference())
     return EmitLoadOfLValue(Constant.getReferenceLValue(*this, E),
                             E->getExprLoc())
         .getScalarVal();
   return Constant.getValue();
 }
 
 llvm::Value *CodeGenFunction::EmitLoadOfScalar(LValue lvalue,
                                                SourceLocation Loc) {
-  return EmitLoadOfScalar(lvalue.getAddress(*this), lvalue.isVolatile(),
+  return EmitLoadOfScalar(lvalue.getAddress(*this),lvalue.isVolatile() |  isVolatileCastStmt,
                           lvalue.getType(), Loc, lvalue.getBaseInfo(),
                           lvalue.getTBAAInfo(), lvalue.isNontemporal());
 }
 
 static bool hasBooleanRepresentation(QualType Ty) {
   if (Ty->isBooleanType())
     return true;
 
LValue CodeGenFunction::EmitCastLValue(const CastExpr *E) {
   case CK_LValueToRValue:
     return EmitLValue(E->getSubExpr());
 
   case CK_NoOp: {
     // CK_NoOp can model a qualification conversion, which can remove an array
     // bound and change the IR type.
     // FIXME: Once pointee types are removed from IR, remove this.
     LValue LV = EmitLValue(E->getSubExpr());
+    // Propagate the volatile qualifer to LValue, if exist in E.
+    if (E->getType().isVolatileQualified())
+      LV.getQuals().addVolatile();
     if (LV.isSimple()) {
       Address V = LV.getAddress(*this);
       if (V.isValid()) {
         llvm::Type *T = ConvertTypeForMem(E->getType());
         if (V.getElementType() != T)
           LV.setAddress(V.withElementType(T));
       }
     }

clang/lib/CodeGen/CGExprScalar.cpp

return CGF.CGM.getTargetCodeGenInfo().performAddrSpaceCast(
         DestTy->getPointeeType().getAddressSpace(), ConvertType(DestTy));
   }
   case CK_AtomicToNonAtomic:
   case CK_NonAtomicToAtomic:
   case CK_UserDefinedConversion:
     return Visit(const_cast<Expr*>(E));
 
   case CK_NoOp: {
+    // Propagate the volatileness if Casted to volatile like
+    //
+    // |-ImplicitCastExpr 'int' <LValueToRValue>
+    // | `-CXXStaticCastExpr 'volatile int' lvalue static_cast<volatile int &> <NoOp>
+    // |   `-ImplicitCastExpr  'volatile int' lvalue <NoOp> part_of_explicit_cast
+    // |     `-DeclRefExpr 'int' lvalue Var 0x55556b04caa0 'x' 'int'
+    //
+    // Later required to emit the load inst accrodingly.
+    if (!CGF.isVolatileCastStmt && CE->getType().isVolatileQualified())
+      CGF.isVolatileCastStmt= true;
+    
     llvm::Value *V = Visit(const_cast<Expr *>(E));
+    
+    // Reset ,so that we don't effect any other address loads.
+    if (CGF.isVolatileCastStmt)
+      CGF.isVolatileCastStmt= false;
     if (V) {
       // CK_NoOp can model a pointer qualification conversion, which can remove
       // an array bound and change the IR type.
       // FIXME: Once pointee types are removed from IR, remove this.
       llvm::Type *T = ConvertType(DestTy);
       if (T != V->getType())
         V = Builder.CreateBitCast(V, T);
     }

clang/lib/CodeGen/CodeGenFunction.h

public:
 
   /// In ARC, whether we should autorelease the return value.
   bool AutoreleaseResult = false;
 
   /// Whether we processed a Microsoft-style asm block during CodeGen. These can
   /// potentially set the return value.
   bool SawAsmBlock = false;
 
+  /// Propagate the volatileness to the reference decl node in the CastStmt
+  /// Note that the decl node may not have the volatile qualifier.
+  bool isVolatileCastStmt = false;
+
   GlobalDecl CurSEHParent;
 
   /// True if the current function is an outlined SEH helper. This can be a
   /// finally block or filter expression.
   bool IsOutlinedSEHHelper = false;
 
   /// True if CodeGen currently emits code inside presereved access index
   /// region.

clang/test/CodeGen/volatile.cpp

+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s -check-prefix CHECK
+
+int x ;
+void f0() {
+    const_cast<volatile int&>(x) = const_cast<volatile int&>(x) + 1;
+//  CHECK: %0 = load volatile i32, ptr @x
+//  CHECK: %add = add nsw i32 %0, 1
+//  CHECK: store volatile i32 %add, ptr @x
+    static_cast<volatile int&>(x) = static_cast<volatile int&>(x) + 1;
+//  CHECK: %1 = load volatile i32, ptr @x
+//  CHECK: %add1 = add nsw i32 %1, 1
+//  CHECK: store volatile i32 %add1, ptr @x
+}