Second try to handle volatile qualifier (corrected)

Nice work. Just a few minor issues: One, the white-space in argument
lists is a bit inconsistent. Second, why does the qualifier list for
MakeAddr default to 0? I can't think of a situation where we'd want
to set a default for the qualifiers like that.

Don't worry about aggregates or complex numbers; I'm thinking it's
likely the aggregate and complex number code will be rewritten to use
the new first-class structs once they're a bit more stable, which will
simplify a lot of the "if complex, else if aggregate, else" sorts of
constructs.

-Eli

Hi,

Here is a more complete implementation. There is style a lot of fixme,
mainly in ObjC and Builtin because I don't know enough about this to do the
right thing. I didn't run the test since I don't think I can do it on
windows (without cygwin). I don't really know how to do testcase, but
attached is a file that I used for testing my change and the result.

Nice work. Just a few minor issues: One, the white-space in argument
lists is a bit inconsistent.

I consistently put a space after a comma except if I need one less char to
fit in 80cols. Is this a problem?
For the return to line, I don't really know how to indent the lines. If you
could point me to things not to do, I could correct them.

Second, why does the qualifier list for
MakeAddr default to 0? I can't think of a situation where we'd want
to set a default for the qualifiers like that.

It was just for testing, and also it mirrored the CreateStore/Load function
which take volatile as an optinal argument. Anyway, corrected.

Don't worry about aggregates or complex numbers; I'm thinking it's
likely the aggregate and complex number code will be rewritten to use
the new first-class structs once they're a bit more stable, which will
simplify a lot of the "if complex, else if aggregate, else" sorts of
constructs.

Some complex code already have some kind of handling of volatile. Didn't
change anything. I let the aggregates mostly alone.

third_try_volatile.patch (23.9 KB)

cg_volatile.c (1.03 KB)

cg_volatile.ll (6.87 KB)

Hi,

Here is a more complete implementation. There is style a lot of fixme,
mainly in ObjC and Builtin because I don't know enough about this to do the
right thing. I didn't run the test since I don't think I can do it on
windows (without cygwin). I don't really know how to do testcase, but
attached is a file that I used for testing my change and the result.

Okay; comments follow.

Nice work. Just a few minor issues: One, the white-space in argument
lists is a bit inconsistent.

I consistently put a space after a comma except if I need one less char to
fit in 80cols. Is this a problem?
For the return to line, I don't really know how to indent the lines. If you
could point me to things not to do, I could correct them.

Oh, it was to fit into 80cols? Then it's okay, I guess.

@@ -644,6 +644,8 @@
     llvm::Type *PtrTy = llvm::PointerType::getUnqual(llvm::Type::Int8Ty);
     Value *One = llvm::ConstantInt::get(llvm::Type::Int32Ty, 1);
     Value *Tmp = Builder.CreateAlloca(llvm::Type::Int32Ty, One, "tmp");
+ // FIXME: in multi threaded application, this should be volatile?
+ // It should also be restrict probably
     Builder.CreateStore(Ops[0], Tmp);
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_sse_ldmxcsr),
                               Builder.CreateBitCast(Tmp, PtrTy));

That FIXME is wrong; the mxcsr register is per-thread, and there's no
point to volatile/restrict qualifying the memory operand.

We do actually have issues relating to re-arranging floating-point
operations around the call, but that's not something we can really
address in clang, so there's no point to having a FIXME there.

@@ -778,6 +782,7 @@
     unsigned Index = BuiltinID == X86::BI__builtin_ia32_loadlps ? 0 : 1;
     llvm::Value *Idx = llvm::ConstantInt::get(llvm::Type::Int32Ty, Index);
     Ops[1] = Builder.CreateBitCast(Ops[1],
llvm::PointerType::getUnqual(EltTy));
+ // FIXME: Volatility
     Ops[1] = Builder.CreateLoad(Ops[1], "tmp");
     Ops[0] = Builder.CreateBitCast(Ops[0], VecTy, "cast");
     Ops[0] = Builder.CreateInsertElement(Ops[0], Ops[1], Idx, "loadps");

There's nothing to fix here; __builtin_ia32_loadlps takes a pointer to
an unqualified vector. Same for the other x86 builtins.

@@ -547,12 +562,14 @@
   }

   FieldDecl *Field = E->getMemberDecl();
- return EmitLValueForField(BaseValue, Field, isUnion);
+ return EmitLValueForField(BaseValue, Field, isUnion,
+ BaseExpr->getType().getCVRQualifiers());
}

Almost, but not quite: for an arrow operator, you care about the
qualifiers on the pointed-to struct, not the pointer.

@@ -598,7 +618,7 @@
   llvm::Value *DeclPtr = CreateTempAlloca(LTy, ".compoundliteral");

   const Expr* InitExpr = E->getInitializer();
- LValue Result = LValue::MakeAddr(DeclPtr);
+ LValue Result = LValue::MakeAddr(DeclPtr,0);

   if (E->getType()->isComplexType()) {
     EmitComplexExprIntoAddr(InitExpr, DeclPtr, false);

What about something like "int a() {return (volatile int){10};}"?
(Not that this is a useful case, but might as well be consistent.)

@@ -640,7 +660,8 @@
LValue CodeGenFunction::EmitCallExprLValue(const CallExpr *E) {
   // Can only get l-value for call expression returning aggregate type
   RValue RV = EmitCallExpr(E);
- return LValue::MakeAddr(RV.getAggregateAddr());
+ // FIXME: can this be volatile?
+ return LValue::MakeAddr(RV.getAggregateAddr(),0);
}

Yes, it can be volatile, although I can't imagine that being of any
use (example: "volatile struct {int x;} a(void);").

@@ -808,6 +808,6 @@
     llvm::InlineAsm::get(FTy, AsmString, Constraints,
                          S.isVolatile() || S.getNumOutputs() == 0);
   llvm::Value *Result = Builder.CreateCall(IA, Args.begin(), Args.end(), "");
- if (ResultAddr)
+ if (ResultAddr) // FIXME: volatility
     Builder.CreateStore(Result, ResultAddr);
}

Hmm, there isn't really any reasonable way to deal with volatile in an
asm statement... we should probably make sure we warn in Sema. Just
leave the fixme for the moment, though.

@@ -418,7 +426,8 @@
       // Initializers can't initialize unnamed fields, e.g. "int : 20;"
       continue;
     }
- LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField, isUnion);
+ // FIXME: Volatile?
+ LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField, isUnion,0);
     if (CurInitVal < NumInitElements) {
       // Store the initializer into the field
       // This will probably have to get a bit smarter when we support

I don't see anything to fix here. (Although volatile aggregates are
going to need a thorough audit once we can reasonably support a
volatile aggregate copy.)

Otherwise, this is looking good! Thanks for putting the time into this.

I'll deal with turning the test into an automated test... although I'm
not completely sure how I'll do it; possibly I'll make it grep for the
correct number of volatile loads and stores.

  // even if the return value of this statement isn't used (like here)
  // clang add a volatile load which can't be removed.
  // is it the correct behavior?

No, it's not really correct; I put in a hacky fix for the somewhat
more serious bug that the assignment expression was coming up with the
wrong value. I'll fix it properly at some point.

-Eli

Hi,

You can also remove:
       // FIXME: Volatility.

I removed the one from EmitStoreThroughLValue, the other are all ObjC
related, so I let them.

Some >80 column violations I think, including:

I don't see any? Some line have just 80cols but I don't think I overflow.

That FIXME is wrong; the mxcsr register is per-thread, and there's no
point to volatile/restrict qualifying the memory operand.

I was thinking about the mxcsr, but if it is saved between context change
then no problem (I never really did x86 assembler, only µC ones)

There's nothing to fix here; __builtin_ia32_loadlps takes a pointer to
an unqualified vector. Same for the other x86 builtins.

Ok, didn't had time to search what these instruction do, so I just put fixme
for more knowledgeable than me to fix

@@ -547,12 +562,14 @@
   }

   FieldDecl *Field = E->getMemberDecl();
- return EmitLValueForField(BaseValue, Field, isUnion);
+ return EmitLValueForField(BaseValue, Field, isUnion,
+ BaseExpr->getType().getCVRQualifiers());
}

Almost, but not quite: for an arrow operator, you care about the
qualifiers on the pointed-to struct, not the pointer.

Corrected and added a case to the test file.

What about something like "int a() {return (volatile int){10};}"?
(Not that this is a useful case, but might as well be consistent.)

The idea was that a literal (compound or not) is a constant expression, and
so don't need to be marked volatile?

Yes, it can be volatile, although I can't imagine that being of any
use (example: "volatile struct {int x;} a(void);").

Ok, done. Don't think it will ever be used.

@@ -418,7 +426,8 @@
       // Initializers can't initialize unnamed fields, e.g. "int :
20;"
       continue;
     }
- LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
isUnion);
+ // FIXME: Volatile?
+ LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
isUnion,0);
     if (CurInitVal < NumInitElements) {
       // Store the initializer into the field
       // This will probably have to get a bit smarter when we support

I don't see anything to fix here. (Although volatile aggregates are
going to need a thorough audit once we can reasonably support a
volatile aggregate copy.)

Since you told me not to bother to much with aggregate, I just put 0 and a
fixme.
Now I removed the fixme.

Otherwise, this is looking good! Thanks for putting the time into
this.

It is not much, I hope to do more (when I find the time and the motivation
simultaneously :)).

  // even if the return value of this statement isn't used (like here)
  // clang add a volatile load which can't be removed.
  // is it the correct behavior?

No, it's not really correct; I put in a hacky fix for the somewhat
more serious bug that the assignment expression was coming up with the
wrong value. I'll fix it properly at some point.

Ok, I removed the comment.

cg_volatile.c (982 Bytes)

fourth_try_volatile.patch (22.9 KB)

What about something like "int a() {return (volatile int){10};}"?
(Not that this is a useful case, but might as well be consistent.)

The idea was that a literal (compound or not) is a constant expression, and
so don't need to be marked volatile?

A "compound literal" is actually an unnamed variable.

@@ -418,7 +426,8 @@
       // Initializers can't initialize unnamed fields, e.g. "int :
20;"
       continue;
     }
- LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
isUnion);
+ // FIXME: Volatile?
+ LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
isUnion,0);
     if (CurInitVal < NumInitElements) {
       // Store the initializer into the field
       // This will probably have to get a bit smarter when we support

I don't see anything to fix here. (Although volatile aggregates are
going to need a thorough audit once we can reasonably support a
volatile aggregate copy.)

Since you told me not to bother to much with aggregate, I just put 0 and a
fixme.
Now I removed the fixme.

Oh! I see the issue. FIXME probably needed. Sorry about that.

Otherwise, this is looking good! Thanks for putting the time into
this.

It is not much, I hope to do more (when I find the time and the motivation
simultaneously :)).

Okay, cool.

I'll fix the last couple issues I just mentioned, fix up the testcase,
and apply the patch soon. Thanks again for doing this.

-Eli