Trying to make Cheerp work with -O0#353
Conversation
andmadri
force-pushed
the
llc-optimizations
branch
from
2dc1461 to
7764bfd
Compare
There was a problem hiding this comment.
I think the idea of checkTrue here is a bit strange. While in javascript, this function does do what its name suggests, it then throws a very specific error (UndefinedMemberAccess), so it cannot be used as a general "checkTrue" function. On the compiler side, the function compileCheckTrue does not do what the name suggests, its argument is not a boolean, and the previous name of compileCheckDefined seems more appropriate.
Taking all of that into consideration, I suggest the javascript function be called checkDefined as it was before, and it takes 2 arguments: The "object" (the bit after in), and the "member" (the bit before in).
Everything else looks good to me, just think checkTrue is a bit confusing at the moment.
llvm/lib/CheerpUtils/I64Lowering.cpp
Outdated
| else | ||
| NewIndices.push_back(Index); | ||
| } | ||
| Value* NewGEP = Builder.CreateGEP(I.getSourceElementType(), I.getPointerOperand(), NewIndices, I.getName()); |
There was a problem hiding this comment.
Style nitpick: you switched to spaces instead of tabs for indentation starting at this line.
There was a problem hiding this comment.
Yes, the name "checkTrue" and throwing "UndefinedMemberAccess" error is weird. I wonder if it is better to change the name to "checkMemberExists" or "checkExists" and throw an error like "InvalidMemberAccess" or "MemberDoesNotExist"? Given that I am using the "in" operator to check if a member exists in the object?
andmadri
force-pushed
the
llc-optimizations
branch
from
7764bfd to
8ae448e
Compare
| void CheerpWriter::compileCheckMemberExistsHelper() | ||
| { | ||
| stream << "function checkDefined(m){if(m===undefined) throw new Error('UndefinedMemberAccess');}" << NewLine; | ||
| stream << "function checkMemberExists(obj, member){if(!member in obj) throw new Error('MemberDoesNotExist');}" << NewLine; |
There was a problem hiding this comment.
The condition in the if statement does not work. It should be if(!(member in obj)).
andmadri
force-pushed
the
llc-optimizations
branch
from
8ae448e to
c6dc557
Compare
| SmallVector< const Value*, 8 > indices(std::next(gep_inst->op_begin()), gep_inst->op_end()); | ||
| Type* basePointedType = cast<GEPOperator>(gep_inst)->getSourceElementType(); | ||
|
|
||
| StructType* containerStructType = dyn_cast<StructType>(GetElementPtrInst::getIndexedType(basePointedType, |
There was a problem hiding this comment.
I am missing something here. Now we actually render the check only for structs, but historically we were also checking for arrays. We should not accidentally lose part of the logic
llvm/lib/CheerpUtils/I64Lowering.cpp
Outdated
| { | ||
| return visitDivRem(I, "__udivti3"); | ||
| } | ||
| HighInt visitGetElementPtrInst(GetElementPtrInst& I) |
There was a problem hiding this comment.
This is not correct. It is arguably completely wrong that there are i64 values in the GEP since the architecture is 32-bit wide.
We should avoid that at the code generation level or alternatively support the case by casting down the bigint at the JS level if needed.
There was a problem hiding this comment.
Currently I am running both the frontend and opt with the -O0 flag. I believe that at the frontend level, whenever we compile with -01 or above, instcombine is called and that lowers any GEP indices from i64 to i32 before they get to our passes and writer.
Because I am not calling instcombine in the frontend nor in the opt passes, some GEP indices remain 64-bit integers and the CheerpWriter represents them as BigInt.
These are some diff of the IR without instcombine and with it.
< %6 = getelementptr inbounds i8*, i8** %1, i64 1
---
> %6 = getelementptr inbounds i8*, i8** %1, i32 1
< call void @_ZN1CC2Ev(%struct._Z1C* noundef nonnull align 1 dereferenceable(8) %6, i8** noundef getelementptr inbounds ([12 x i8*], [12 x i8*]* @_ZTT1D, i32 0, i64 5)) #4
---
> call void @_ZN1CC2Ev(%struct._Z1C* noundef nonnull align 1 dereferenceable(8) %6, i8** noundef nonnull getelementptr inbounds ([12 x i8*], [12 x i8*]* @_ZTT1D, i32 0, i32 5)) #4
When I talked to Yuri about it, we thought that a good solution would be implementing something in the I64Lowering pass rather than changing the writer. I can instead try to support this case by casting them down at the js level.
There was a problem hiding this comment.
When I mean frontend I am talking about frontend optimization, but the original code generation from C++. I believe it's not correct for the i64 to be there from the very beginning.
I am not sure what was Yuri's reasoning, but I don't agree with the use of I64Lowering. The code is not lowering the value, it's truncating it.
There was a problem hiding this comment.
I understand. You are right I remember he did say truncating when I showed him the code. Maybe it is a stupid question but could the solution be never running the frontend with -O0?
There was a problem hiding this comment.
Nothing stupid in the question, but O0 can certainly be used in frontend. It can be made the work.
Before optimizations even start the code is already wrong. There is no reason to use 64-bit offsets on a 32-bit platform. Clang itself is generated code poorly, it's not uncommon.
The previous logic of checkDefined was causing issues when compiling with the no-optimization flag together with -cheep-bounds-check. Instead of checking if a value is undefined and throwing an error, it is better to check if the member of an object exists.
andmadri
force-pushed
the
llc-optimizations
branch
from
c6dc557 to
9da6fcf
Compare
3 participants
I made several changes all realted to making -O0 work with our test suite.
The change to GDA concerned making the configure part of Ruby to be a bit faster. The idea is that it will still fail but it will not dump a core.
In CheerpWriter I changed the checkDefined function to a checkTrue function.
I had assertions because BigInts were being mixed with Numbers in the same expressions for the js files. The fix was to lower i64 getelementptr indices during the I64lowering pass.