[NativeAOT-LLVM] For TARGET_WASM do not expand to GT_INDEX_ADDR #1824
Conversation
|
cc @SingleAccretion (had @ in the wrong place) |
src/coreclr/jit/llvm.cpp
Outdated
| void Llvm::buildIndexAddr(GenTreeIndexAddr* indexAddr) | ||
| { | ||
| Value* firstElementAddr = | ||
| _builder.CreateGEP(getGenTreeValue(indexAddr->gtGetOp1()), _builder.getInt32(indexAddr->gtElemOffset)); | ||
| Value* offset = _builder.CreateMul(_builder.getInt32(indexAddr->gtElemSize), getGenTreeValue(indexAddr->Index())); | ||
|
|
||
| mapGenTreeToValue(indexAddr, _builder.CreateGEP(firstElementAddr, offset)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm, this won't work as-is, INDEX_ADDR is effectively bounds-checked (and we're missing the bounds check).
I think it would be better to just always fully expand INDEX in morph for LLVM, INDEX_ADDR's only purpose is to increase throughput under MinOpts, and I think that sort of tuning can wait (and is probably not worthwhile anyway).
There was a problem hiding this comment.
I see you've got a PR in runtime around this area. Would you prefer this was left until that was accepted and merged here?
There was a problem hiding this comment.
I don't think there is a need to block this change on my changes in the runtime.
The fix here should be very simple: #ifdef-out the logic that creates INDEX_ADDRs for TARGET_WASM in fgMorphArrayIndex.
There was a problem hiding this comment.
thanks, have #if'ed it out.
yowl
changed the title
This PR adds support in the LLVM clrjit for the
GT_INDEX_ADDRinstruction. Do I need some todo's in here for 64 bit Wasm on either the index, the element offset (unlikely I suppose, its just the type + length I think), or the element size?@cc SingleAccretion.
Thanks,