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 ↗

(On Diff #549964)

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

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

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

3639

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

2230

Consistent indentation, please.

[umesh.kalappa0]

clang/include/clang/AST/Expr.h

3639

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

3639

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

3639

@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

3639

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

[rjmccall]

clang/include/clang/AST/Expr.h

3639

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

3639

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

3636

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

3636

@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/include/clang/AST/Expr.h

protected:
   const FPOptionsOverride *getTrailingFPFeatures() const {
     return const_cast<CastExpr *>(this)->getTrailingFPFeatures();
   }
 
 public:
   CastKind getCastKind() const { return (CastKind) CastExprBits.Kind; }
   void setCastKind(CastKind K) { CastExprBits.Kind = K; }
 
+  /// Return 
+  //  True :  if the cast expr has the volatileness in the cast node path and 
+  //          declrefexpr doesn't has the volatile qualfiler like
+  //  |-ImplicitCastExpr 'int' <LValueToRValue>
+  //  | `-CXXStaticCastExpr 'volatile int' lvalue static_cast<volatile int &>
+  //  |   `-ImplicitCastExpr  'volatile int' lvalue <NoOp> part_of_explicit_cast
+  //  |     `-DeclRefExpr 'int' lvalue Var 'x' 'int'
+  //  False :  Otherwise.
+  bool isWorthToPropagateVolatile(CastExpr *CE) const {
+    if (CE->getCastKind() != CK_NoOp )   
+       return (this->isGLValue() && 
+               this->getType().isVolatileQualified() != 
+         CE->getType().isVolatileQualified());
+    return isWorthToPropagateVolatile(static_cast<CastExpr *>(CE->getSubExpr()));
+  }
+
+
   static const char *getCastKindName(CastKind CK);
   const char *getCastKindName() const { return getCastKindName(getCastKind()); }
 
   Expr *getSubExpr() { return cast<Expr>(Op); }
   const Expr *getSubExpr() const { return cast<Expr>(Op); }
   void setSubExpr(Expr *E) { Op = E; }
 
   /// Retrieve the cast subexpression as it was written in the source
public:
   static const FieldDecl *getTargetFieldForToUnionCast(QualType unionType,
                                                        QualType opType);
   static const FieldDecl *getTargetFieldForToUnionCast(const RecordDecl *RD,
                                                        QualType opType);
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() >= firstCastExprConstant &&
            T->getStmtClass() <= lastCastExprConstant;
   }
 
   // Iterators
   child_range children() { return child_range(&Op, &Op+1); }
   const_child_range children() const { return const_child_range(&Op, &Op + 1); }
 };
 
 /// ImplicitCastExpr - Allows us to explicitly represent implicit type
 /// conversions, which have no direct representation in the original
 /// source code. For example: converting T[]->T*, void f()->void
 /// (*f)(), float->double, short->int, etc.
 ///

clang/lib/CodeGen/CGExpr.cpp

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/CGExprComplex.cpp

public:
   ComplexPairTy VisitImplicitCastExpr(ImplicitCastExpr *E) {
     // Unlike for scalars, we don't have to worry about function->ptr demotion
     // here.
     return EmitCast(E->getCastKind(), E->getSubExpr(), E->getType());
   }
   ComplexPairTy VisitCastExpr(CastExpr *E) {
     if (const auto *ECE = dyn_cast<ExplicitCastExpr>(E))
       CGF.CGM.EmitExplicitCastExprType(ECE, &CGF);
+    if (E->getCastKind() == CK_NoOp &&
+        E->isWorthToPropagateVolatile(static_cast<CastExpr *>(E->getSubExpr())))
+       return EmitLoadOfLValue(E);
     return EmitCast(E->getCastKind(), E->getSubExpr(), E->getType());
   }
   ComplexPairTy VisitCallExpr(const CallExpr *E);
   ComplexPairTy VisitStmtExpr(const StmtExpr *E);
 
   // Operators.
   ComplexPairTy VisitPrePostIncDec(const UnaryOperator *E,
                                    bool isInc, bool isPre) {

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: {
-    llvm::Value *V = Visit(const_cast<Expr *>(E));
+    llvm::Value *V = CE->isWorthToPropagateVolatile(static_cast<CastExpr *>(E))
+                     ? EmitLoadOfLValue(CE)
+         : Visit(const_cast<Expr *>(E)) ;
     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);
     }
     return V;
   }
 
   case CK_BaseToDerived: {
     const CXXRecordDecl *DerivedClassDecl = DestTy->getPointeeCXXRecordDecl();
     assert(DerivedClassDecl && "BaseToDerived arg isn't a C++ object pointer!");
 
     Address Base = CGF.EmitPointerWithAlignment(E);
     Address Derived =
       CGF.GetAddressOfDerivedClass(Base, DerivedClassDecl,

clang/test/CodeGen/volatile.cpp

+// RUN: %clang_cc1 -O2 -triple=x86_64-unknown-linux-gnu -emit-llvm %s -o -  | FileCheck %s -check-prefix CHECK
+struct agg 
+{
+int a ;
+int b ;
+} t;
+struct agg a;
+int vt;
+_Complex float cf;
+
+void f0() {
+    const_cast<volatile _Complex float &>(cf) = const_cast<volatile _Complex float&>(cf) + 1;
+//  CHECK: %cf.real = load volatile float, ptr @cf
+//  CHECK: %cf.imag = load volatile float, ptr getelementptr
+//  CHECK: %add.r = fadd float %cf.real, 1.000000e+00
+//  CHECK: %add.i = fadd float %cf.imag, 0.000000e+00
+//  CHECK: store volatile float %add.r
+//  CHECK: store volatile float %add.i, ptr getelementptr
+      static_cast<volatile _Complex float &>(cf) = static_cast<volatile _Complex float&>(cf) + 1;
+//  CHECK: %cf.real1 = load volatile float, ptr @cf
+//  CHECK: %cf.imag2 = load volatile float, ptr getelementptr
+//  CHECK: %add.r3 = fadd float %cf.real1, 1.000000e+00
+//  CHECK: %add.i4 = fadd float %cf.imag2, 0.000000e+00
+//  CHECK: store volatile float %add.r3, ptr @cf
+//  CHECK: store volatile float %add.i4, ptr getelementptr
+    const_cast<volatile  int  &>(a.a) = const_cast<volatile int &>(t.a) ;
+//  CHECK: %0 = load volatile i32, ptr @t
+//  CHECK: store volatile i32 %0, ptr @a
+    static_cast<volatile  int  &>(a.b) = static_cast<volatile int  &>(t.a) ;
+//  CHECK: %1 = load volatile i32, ptr @t
+//  CHECK1: store volatile i32 %1, ptr getelementptr
+    const_cast<volatile int&>(vt) = const_cast<volatile int&>(vt) + 1;
+//  CHECK: %2 = load volatile i32, ptr @vt
+//  CHECK: %add = add nsw i32 %2, 1
+//  CHECK: store volatile i32 %add, ptr @vt
+     static_cast<volatile int&>(vt) = static_cast<volatile int&>(vt) + 1;
+//  CHECK: %3 = load volatile i32, ptr @vt
+//  CHECK: %add5 = add nsw i32 %3, 1
+//  CHECK: store volatile i32 %add5, ptr @vt
+}