This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv Core 08/14] Do not simplify expressions with FPEnv access
AcceptedPublic

Authored by sdmitrouk on Oct 26 2015, 6:56 AM.

Details

Summary

Modify ConstantExpression to store these flags and then use them.

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitrouk updated this revision to Diff 38406.Oct 26 2015, 6:56 AM
sdmitrouk retitled this revision from to [FPEnv Core 08/14] Do not simplify expressions with FPEnv access.
sdmitrouk updated this object.
sdmitrouk added reviewers: hfinkel, mehdi_amini.
sdmitrouk set the repository for this revision to rL LLVM.
sdmitrouk added subscribers: llvm-commits, resistor, scanon.
sdmitrouk updated this revision to Diff 41917.Dec 4 2015, 12:09 PM

Update of test per flags rename and correction of one condition (need to ignore non-floating-point types).

sdmitrouk updated this revision to Diff 42534.Dec 11 2015, 9:40 AM

Use different overload of ConstantExpr::get().

mehdi_amini added inline comments.Dec 11 2015, 3:59 PM
include/llvm/IR/Constants.h
1107

Document the new parameter

sdmitrouk updated this revision to Diff 42723.Dec 14 2015, 7:59 AM
sdmitrouk marked an inline comment as done.

Document the new parameter (Strict).

mehdi_amini accepted this revision.Dec 14 2015, 11:10 AM
mehdi_amini edited edge metadata.
This revision is now accepted and ready to land.Dec 14 2015, 11:10 AM
majnemer added inline comments.
lib/IR/Constants.cpp
1891–1892

Er, why not teach ConstantExpression about these flags?

mehdi_amini requested changes to this revision.Dec 14 2015, 11:39 AM
mehdi_amini edited edge metadata.

(mark unaccepted waiting to settle the ConstantExpr case)

lib/IR/Constants.cpp
1891–1892

I thought that since it was float specific it couldn't be added, but I just checked and saw that we have integer nsw/nuw already on ConstantExpr, so it seems possible.

ConstantExprKeyType accepts unsigned short SubclassOptionalData. FastMathFlags are currently unsigned Flags but there is not reason they couldn't be uint16_t instead right?

David: do you know why we don't already have FastMath on ConstantExpr? Is it because there was no case with FP where we wouldn't constant fold immediately?

Now, what are the potential benefit to deal with Floating point ConstantExpr with FastMath flags? And what are the complications (i.e. will we have to add extra care everywhere we deal with FP ConstantExpr?)?

This revision now requires changes to proceed.Dec 14 2015, 11:39 AM
majnemer added inline comments.Dec 14 2015, 11:41 AM
lib/IR/Constants.cpp
1891–1892

AFAIK, yes. We didn't believe the flags would meaningfully change the result of what expression we'd give back. This would change with the new flags.

mehdi_amini added inline comments.Dec 14 2015, 1:25 PM
lib/IR/Constants.cpp
1891–1892

Talk with David on IRC, I agree with him that it seems cleaner to have this.
Sergey: can you prepare a separate patch that would add FMF to ConstantExpr?
Thanks!

sdmitrouk added inline comments.Dec 14 2015, 1:42 PM
lib/IR/Constants.cpp
1891–1892

I forgot about this change and another code related to constant expressions got removed from different patch, so I thought there is no need to do something about them.

can you prepare a separate patch that would add FMF to ConstantExpr?

I can try if it's just adding the field and considering it in several places where folding is performed. If it's significantly more complex, then I'm afraid someone else should do this as I'll stop contributing to LLVM very soon (and either upload the rest of the patches for ARM and FENV pragma in Clang or pass them over to somebody else to do this).

By the way, the field is not of unsigned short type:

uint8_t SubclassOptionalData;
sdmitrouk updated this revision to Diff 42925.Dec 15 2015, 3:18 PM
sdmitrouk edited edge metadata.

Replacing with FastMathFlags for ConstantExpr, separate patch would almost completely undo this one and two more (two following ones, which are related to Strict flag), so I guess it's better to have it here.

Realized that I forgot to update other methods of ConstantExprKeyType to include the new field, will do that later, it won't change much. Optional data field was already used and it's only 7bits in Value class, which would need to be extended to hold new flags, so I added a field to ConstantExpr instead.

sdmitrouk updated this revision to Diff 43011.Dec 16 2015, 8:02 AM
sdmitrouk edited edge metadata.
mehdi_amini added inline comments.Dec 18 2015, 4:13 PM
lib/AsmParser/LLParser.cpp
2985 ↗(On Diff #43011)

Remove

lib/IR/ConstantsContext.h
423 ↗(On Diff #43011)

Can we reuse one of the previous field?

sdmitrouk updated this revision to Diff 43352.Dec 21 2015, 2:50 AM

Remove extra empty line.

sdmitrouk marked an inline comment as done.Dec 21 2015, 2:50 AM
sdmitrouk added inline comments.
lib/IR/ConstantsContext.h
423 ↗(On Diff #43011)

In case of binary expressions SubclassData holds 0 and SubclassOptionalData holds binary operation specific flags. We can use SubclassData, which will be inconsistent with other types of constant expressions, but can be done if that's OK.

mehdi_amini added inline comments.Jan 2 2016, 7:48 PM
lib/Analysis/ConstantFolding.cpp
1009

Why not always use CE->getFastMathFlags();? Wasn't the FMF param added to this function because of the lack of FMF in ConstantExpr originally?

lib/IR/ConstantsContext.h
423 ↗(On Diff #43352)

I think it is OK, unless you have any clue why it wouldn't?

sdmitrouk added inline comments.Jan 23 2016, 6:26 AM
lib/Analysis/ConstantFolding.cpp
1009

Wasn't the FMF param added to this function because of the lack of FMF in ConstantExpr originally?

No, it wasn't. FMF can be from instruction which contains CE as one of its operands, the argument was added for llvm::ConstantFoldInstruction.

lib/IR/ConstantsContext.h
423 ↗(On Diff #43352)

No, I just didn't see such use and thought maybe there is a reason for that, which isn't documented. I'll change this then.

mehdi_amini added inline comments.Jan 23 2016, 11:03 AM
lib/Analysis/ConstantFolding.cpp
1009

Again, now that constant expression have dedicated FMF, I'm not sure why we should apply the parent instruction FMF?
If a constant expression does not have FMF enabled and it an operand of an instruction with FMF enabled it does not give license to fold I think.
And on the opposite, a constant expression with FMF as an operand of an instruction with FMF disabled should be foldable.

It is very possible that I miss something, David and/or Hal may have an opinion on this?

sdmitrouk added inline comments.Jan 23 2016, 11:34 AM
lib/Analysis/ConstantFolding.cpp
1009

Maybe we can drop it, I'll need to check (don't remember by now) and see whether it causes any problems. Initially it did make sense to propagate flags of instruction to its operands, I agree that it might be useless now.

sdmitrouk updated this revision to Diff 45876.Jan 25 2016, 10:25 AM
sdmitrouk updated this object.

Reuse SubclassData and remove FastMathFlags argument.

mehdi_amini accepted this revision.Jan 25 2016, 10:32 AM
mehdi_amini edited edge metadata.

LGTM. Thanks.

This revision is now accepted and ready to land.Jan 25 2016, 10:32 AM